Skip to content

Conversation

@scampi
Copy link
Contributor

@scampi scampi commented May 2, 2017

close #11575

@elasticmachine
Copy link
Contributor

Can one of the admins verify this patch?

@scampi scampi changed the title disable save button if visualization is dirty [WIP] disable save button if visualization is dirty May 3, 2017
@scampi
Copy link
Contributor Author

scampi commented May 3, 2017

I'm going to try to write a small test for this.

@scampi scampi changed the title [WIP] disable save button if visualization is dirty disable save button if visualization is dirty May 3, 2017
@scampi
Copy link
Contributor Author

scampi commented May 3, 2017

PR is now ready for review.

@tbragin tbragin added the Feature:Visualizations Generic visualization features (in case no more specific feature label is available) label May 4, 2017
@thomasneirynck thomasneirynck self-requested a review May 12, 2017 16:12
@thomasneirynck
Copy link
Contributor

hi @scampi, my apologies for the little delay on this one. I'll take a look later today. In the meantime, do you mind rebasing and resolve merge conflict when you get a chance? thx!

Copy link
Contributor

@thomasneirynck thomasneirynck left a comment

Choose a reason for hiding this comment

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

This is a very nice UX improvement. thx!

The merge conflict is pretty minor, some other methods were inserted at the same location in the UI-test helpers, so rebase should be pretty straightforward.

LGTM pending rebase.

Copy link
Contributor

Choose a reason for hiding this comment

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

this is why you're awesome @scampi. Community PRs rarely include UI-tests!!!

@thomasneirynck thomasneirynck requested a review from ppisljar May 12, 2017 17:56
@thomasneirynck
Copy link
Contributor

@ppisljar you want to take 2nd look? thx

@scampi
Copy link
Contributor Author

scampi commented May 12, 2017

@thomasneirynck thanks for the kind words! I have rebased against master. I added a little 3rd commit so that the tests make more sense.

Copy link
Contributor

@ppisljar ppisljar left a comment

Choose a reason for hiding this comment

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

LGTM

@thomasneirynck thomasneirynck merged commit efc6218 into elastic:master May 16, 2017
thomasneirynck pushed a commit to thomasneirynck/kibana that referenced this pull request May 16, 2017
(backport: manual merge edits in test file)
@thomasneirynck
Copy link
Contributor

5.x backport: #11816

@scampi scampi deleted the visualize-save branch May 16, 2017 14:27
thomasneirynck added a commit that referenced this pull request May 16, 2017
(backport: manual merge edits in test file)
snide pushed a commit to snide/kibana that referenced this pull request May 30, 2017
@thomasneirynck
Copy link
Contributor

I'm going to revert this as it introduced two UX regressions (#12082). I missed those. I think this is a pretty good change still, but we'll need to look into two issues:

  • changing the time-filter should not cause the save-button to be disabled
  • disabling the save button prevents users from doing a save-to-rename operation

thomasneirynck added a commit to thomasneirynck/kibana that referenced this pull request Jun 2, 2017
thomasneirynck added a commit that referenced this pull request Jun 12, 2017
thomasneirynck added a commit to thomasneirynck/kibana that referenced this pull request Jun 12, 2017
thomasneirynck added a commit to thomasneirynck/kibana that referenced this pull request Jun 12, 2017
thomasneirynck added a commit that referenced this pull request Jun 13, 2017
thomasneirynck added a commit that referenced this pull request Jun 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Feature:Visualizations Generic visualization features (in case no more specific feature label is available) release_note:fix review v5.5.0 v6.0.0-alpha2 v6.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

save button of the visualize page should be disabled if visualization is dirty

6 participants