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

[EuiSuperDatePicker] “Now” tab titled “Set to Now” #1620

Merged
merged 4 commits into from
Mar 8, 2019

Conversation

cchaos
Copy link
Contributor

@cchaos cchaos commented Feb 27, 2019

Fixes #1598 by changing the wording of the tab.

Checklist

  • [ ] This was checked in mobile
  • [ ] This was checked in IE11
  • [ ] This was checked in dark mode
  • [ ] Any props added have proper autodocs
  • [ ] Documentation examples were added
  • A changelog entry exists and is marked appropriately
  • [ ] This was checked for breaking changes and labeled appropriately
  • [ ] Jest tests were updated or added to match the most common scenarios
  • [ ] This was checked against keyboard-only and screenreader scenarios
  • [ ] This required updates to Framer X components

@cchaos cchaos requested a review from formgeist February 27, 2019 19:38
@formgeist
Copy link
Contributor

@cchaos Rewording helps a bit, but in my opinion this option is different than Absolute / Relative, in that those two translate each other, whereas Now destroys any date input you previously made.

You can argue that relative translates Now, but not much help for the user as they will almost certainly have to start over. The expectations set in Absolute and Relative don't match the "Set to now". I'd much prefer to have it in the Relative pane because as you say, it's = 0 seconds ago.

Would it make sense to put it in the Relative pane by the date input field, i.e.:

date-picker-now

The description in the Now pane could be displayed as a tooltip with a delay "Set to now" means that on every refresh this time will be set to the time of the refresh.

Alternatively, adding a button to the Now pane after the description, basically like an Apply button in the Quick select.

date-picker-now copy

@cchaos
Copy link
Contributor Author

cchaos commented Mar 7, 2019

@formgeist We've decided to go with the button options because adding the The "Set to now" text on the relative input doesn't really describe what the "now" time means. We still think the description in the tab is valuable.

I've updated the description of the PR, can you take another look?

@nreese I'm adding you as a PR reviewer. If you don't mind checking over my onChange event handlers and making sure that the Kibana implementation will be good with it.

@cchaos cchaos requested a review from nreese March 7, 2019 18:18
@cchaos
Copy link
Contributor Author

cchaos commented Mar 7, 2019

Hmmm, I seem to have broken the update function??

@nreese
Copy link
Contributor

nreese commented Mar 7, 2019

My only concern with this change is it makes the now tab behave differently then the other tabs. When you hit the absolute or relative tabs, the date is automatically updated to be absolute or relative. With this change, the now tab is different and requires the user to click a button. Should we also add buttons to the other tabs or wait until the user makes a selection to then switch the time to absolute or relative?

Copy link
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

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

code changes lgtm as long as everyone is happy with the fact that switching tabs has incosistent behavior across absolute, relative, and now

@cchaos
Copy link
Contributor Author

cchaos commented Mar 7, 2019

That's correct. We understand that just switching to the Now tab does nothing until you hit the button. It allows for more discoverability of the option without losing the previous selection since switching between relative and absolute stays around the selected date, while the now tab would change the selected date.

Any thoughts on why the update button would not update correctly anymore?

@nreese
Copy link
Contributor

nreese commented Mar 7, 2019

Any thoughts on why the update button would not update correctly anymore?

that is actually a bug in master

@cchaos
Copy link
Contributor Author

cchaos commented Mar 7, 2019

Ohh it's on master. I checked the published docs so I thought it was my fault. Ok. Do we have an issue or PR for it?

@nreese
Copy link
Contributor

nreese commented Mar 7, 2019

Ok. Do we have an issue or PR for it?

I have not seen one. First time I have seen the issue

@cchaos
Copy link
Contributor Author

cchaos commented Mar 7, 2019

hmm, I'll check the change log. Thanks @nreese !

@cchaos
Copy link
Contributor Author

cchaos commented Mar 7, 2019

Mysteriously, it's working again....

Copy link
Contributor

@formgeist formgeist left a comment

Choose a reason for hiding this comment

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

LGTM 👍Much calmer introduction of the Now option.

@cchaos cchaos merged commit 255d330 into elastic:master Mar 8, 2019
@cchaos cchaos deleted the set-to-now branch March 8, 2019 14:53
cchaos pushed a commit to cchaos/eui that referenced this pull request Mar 9, 2019
ryankeairns pushed a commit to ryankeairns/eui that referenced this pull request Mar 11, 2019
cchaos pushed a commit to ryankeairns/eui that referenced this pull request Mar 11, 2019
…rigger the "now" time selection (elastic#1620)""

This reverts commit d74f18f.
Shigawire pushed a commit to Shigawire/eui that referenced this pull request May 10, 2019
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