Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't mark YAML editor as immediately changed #1666

Merged
merged 1 commit into from
Jun 8, 2017

Conversation

spadgett
Copy link
Member

@spadgett spadgett commented Jun 8, 2017

This fixes two issues:

  • Don't enable the save button before the user modifies the YAML. The intent in the code is for the button to only enable after the user types something.
  • Don't prompt about unsaved changes when the user navigates away immediately.

@spadgett spadgett requested a review from jwforres June 8, 2017 13:35

// Wait for the editor to initialize before adding the on change handler
// so it's not immediately marked as modified.
setTimeout(function() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this always going to work or are we creating a race condition where it will just work most of the time

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't find a cleaner way. It always works in my testing.

We could do something like this:

    // onChange is called once when the initial content is loaded in the editor.
    var initialized = false;
    $scope.onChange = function() {
      if (initialized) {
        $scope.modified = true;
      }

      initialized = true;
    };

I worry that this will break if they ever change the behavior so that initialization doesn't trigger onChange, however.

Open to alternatives. The worst that happens is that the editor is incorrectly marked dirty so you're prompted when pressing the back button.

@jwforres
Copy link
Member

jwforres commented Jun 8, 2017

[merge]

@openshift-bot
Copy link

Evaluated for origin web console merge up to d39d160

@openshift-bot
Copy link

openshift-bot commented Jun 8, 2017

Origin Web Console Merge Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_web_console/1487/) (Base Commit: 7a49dad)

@openshift-bot openshift-bot merged commit 23aa410 into openshift:master Jun 8, 2017
@spadgett spadgett deleted the edit-yaml-on-change branch June 8, 2017 19:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants