[ZEPPELIN-3814] visualization - Add apply button to table settings #3205
Conversation
| <div type="button" ng-click="applyTableOption()" | ||
| uib-tooltip="Apply new setting" tooltip-placement="left" | ||
| class="btn btn-default" style="font-size: 11px; padding: 2px 5px 2px 5px;"> | ||
| <i class="fa fa-check-square" aria-hidden="true"></i> |
There was a problem hiding this comment.
this looks useful but a checked box button might be a bit confusing as "check all box" perhaps?
There was a problem hiding this comment.
Maybe "save" button or "check" button would be better, WDYT? In my opinion, checked box button is better :)
- Save
- Check
There was a problem hiding this comment.
ping @felixcheung
There was a problem hiding this comment.
@felixcheung Look again, please :)
There was a problem hiding this comment.
hmm, I guess either is ok :)
so I'm wondering with this behavior change, would it be easy to forget to "apply"? would it be possible to pop a tooltip when anything is changed to direct user to click "apply all"?
There was a problem hiding this comment.
@felixcheung table settings have just three options, so I suppose it's not really necessary, moreover this behavior is not supported in more complexity settings in advanced-transformation-settings.
There was a problem hiding this comment.
could you clarify? I'm not suggest to change any behavior - just pop a tooltip when one of the three option is changed, since, if I understand, any change applies immediately. now, with your change, it would seem to the user that "it doesn't do anything"
?
There was a problem hiding this comment.
Now changes apply only after page refreshing, my PR saves this logic, but also provides button with apply changes function. As for me, this tooltip is not necessary, because there are only three options in list.
There was a problem hiding this comment.
your screencast shows multiple changes are applied only after save is clicked, and it refreshes the visual. am I understanding you right?
https://user-images.githubusercontent.com/6136993/51320121-ecea9780-1a6f-11e9-850f-fcaf061ff5f3.gif
There was a problem hiding this comment.
I just tried to show the new functionality, you can also just refresh the page after clicking on the checkboxes, and the changes will be applied.
|
I decided to put floppy icon as in @jongyoul please help reviewing this.
|
|
@felixcheung could you merge changes or give some feedback?? |
|
hey sorry about missing this. I'm going to check this PR now if you'd like to see my comment (here #3205 (comment)). |
|
could you also update the images in the PR description to reflect the final visual (these gets linked to in the commit later) #3205 (comment) |
|
Thank you for review, description is updated. |
felixcheung
changed the title
|
I updated the PR title - hopeful someone might look at it. |
|
kicking off tests |
|
Time to merge?) |
What is this PR for?
Now changes in table settings applies only after page refreshing which isn't convenient
What type of PR is it?
Improvement
What is the Jira issue?
How should this be tested?
Questions: