EuiSuperDatePicker #1351

nreese · 2018-12-06T21:10:25Z

Summary

This PR replaces the globalDatePicker pattern with a fully functional EuiSuperDatePicker component that can be used as the Kibana global date picker.

Checklist

…ker props

cchaos · 2018-12-06T21:12:24Z

Wowsah! Can't wait to go through this one. I'll probably make this a quick pass tomorrow and anything that's just style related can be a follow-up PR so that this can unblock your Kibana implementation.

cchaos · 2018-12-07T15:58:40Z

It's so fun to see it all hooked up. Thanks so much for doing this!

Visual check

When showing the pretty format, can you spell out the full thing so that this says "Last 15 days" instead of "Last 15d"

Basically anything they input from the quick select form should read out the same in the input. For instance, I applied Next / 160 / minutes from the quick select but the input then read now to ~ in 3 hours

In the quick select, can you persist the last edit to that form? It always seems to revert back to Last / 15 / minutes
Recently used date ranges should populate from the top not the bottom, meaning the most recent is at the top.
When using the next / previous time window arrow, I think it's ok to keep the popover open so they can quickly keep shifting that window.
After moving the time window, since you are showing "[date] to [date]" should it not be a pretty format but the actual date inputs?

Oh I see, that when I click "Show dates" it reverts to the date inputs, but then when I hit refresh, it goes back to the "[date] to [date]" format. Can we persist that any time it wants to show a "[date] to [date]" format, it just uses the date inputs? Having to keep clicking "Show dates" to modify these actual dates is quite cumbersome.

I think the main function of the time picker should be that by default it's always the two date inputs unless they use the quick select form from the popover.

It will also do a better job at displaying the full dates since the pretty format of "[date] to [date]" hardly shows much of the end date.

vs.

Even when using the Relative picker per date, it seems odd to me that on Refresh it turns it into a string instead of keeping the date inputs there.

I also find it harder to read with using "to" instead of the arrow and the spacing around each date.

Is now to now valid? It's not showing as invalid.

If any of that is confusing or you want to talk it through again on zoom, just ping me. I'll start a code review now.

cchaos · 2018-12-07T16:29:21Z

Another issue I just saw was that, other than the "__ to date" commonly used options, the rest of them (when showing their dates) don't look correct. It seems to be passing the same values to 'start' and 'end':

…w, now to now isInvalid, recenlty selected in reverse order

cchaos

I just concentrated the code review on the actual components and will leave the more complicated logic/utility bits for @chandlerprall to review :)

My comments were all mostly small things and can probably change after the fact (another PR). My main concerns are the ones listed above when it comes to interaction and the pretty format state.

src/components/date_picker/_index.scss

src/components/date_picker/index.js

src/components/date_picker/super_date_picker/date_popover/absolute_tab.js

cchaos · 2018-12-07T16:32:00Z

src/components/date_picker/super_date_picker/super_date_picker.js

+
+EuiSuperDatePicker.defaultProps = {
+  from: 'now-15m',
+  to: 'now',


This may too much work, but I find 'start' and 'end' to be more understandable than 'from' and 'to'. Maybe we could change them?

Maybe let's just change these as the prop names for the larger component and not worry about the sub-components for now. But at least the consumer will only have knowledge of start and end needs.

src/components/date_picker/super_date_picker/quick_select_popover/commonly_used.js

src/components/date_picker/super_date_picker/date_popover/relative_tab.js

src/components/date_picker/super_date_picker/date_popover/date_popover_button.js

src-docs/src/views/date_picker/super_date_picker.js

src/components/date_picker/super_date_picker/super_date_picker.js

src/components/date_picker/super_date_picker/pretty_duration.js

…urate, make onRefreshChange prop optional

nreese · 2018-12-10T20:58:05Z

@snide @cchaos That should be the last of the requested changes

cchaos · 2018-12-10T21:31:46Z

Looks great, though I found one more thing :sorry:

When using the relative tab and I select try to change the units first (while quantity is still 0), it won't change. Can you let it accept any unit even when it's set to 0?

cchaos

Ok I did a final review and tested in IE11 and Firefox. There are a few issues in each but nothing major. I'll still be doing a follow up design PR anyway, so I can take a look into those. But I think we're good to merge!

Thank you so much @nreese! This is definitely one of the best components we've built!

snide

Nice work @nreese!

snide · 2018-12-12T18:34:28Z

Is this mergable @nreese? Actually got a request from @justinkambic to use it in something he's got for 6.6 (I know it won't get into the global scope, this is something different he's working on). I could likely get it into another PR I have going today for Kibana if so, then he could use it.

nreese · 2018-12-12T19:05:11Z

@snide I was waiting on a review from @chandlerprall before merging but if you think it should merge as-is, I can always address his feedback in a separate PR

snide · 2018-12-12T19:06:20Z

No. I just totally forgot we had that part to do still. No worries.

package.json

src/components/date_picker/super_date_picker/date_popover/absolute_tab.js

src/components/date_picker/super_date_picker/date_popover/date_popover_button.js

src/components/date_picker/super_date_picker/quick_select_popover/refresh_interval.js

chandlerprall · 2018-12-12T22:48:49Z

src/components/date_picker/super_date_picker/quick_select_popover/refresh_interval.js

+    };
+  }
+
+  static getDerivedStateFromProps = (nextProps) => {


Same thing here, overriding state

Overriding the state is intended. The point is that when the props update, the state must update to reflect what is getting passed in.

src/components/date_picker/super_date_picker/relative_utils.js

chandlerprall · 2018-12-12T22:52:34Z

src/components/date_picker/super_date_picker/super_date_picker.js

+    };
+  }
+
+  static getDerivedStateFromProps = (nextProps) => {


Unfortunately, same case here

Overriding the state is intended. The point is that when the props update, the state must update to reflect what is getting passed in.

src/components/date_picker/super_date_picker/super_date_picker.js

…ab, EuiRefreshInterval, and EuiRelativeTab

nreese · 2018-12-14T21:32:04Z

@chandlerprall After passing selected to EuiDatePicker in EuiAbsoluteTab, the following warning is displayed in the console. Any ideas why the proptypes check is failing?

nreese · 2018-12-17T15:29:38Z

@chandlerprall This is ready for a another look

XavierM · 2018-12-17T16:30:42Z

@nreese Should we also introduce kibana localization/i18n to this PR?

nreese · 2018-12-17T17:36:44Z

@nreese Should we also introduce kibana localization/i18n to this PR?

No, EUI is not ready to handle i18n yet but there is an open issue, #738. This component will need to be localized once that issue is resolve and an implementation is known

chandlerprall

Changes LGTM! Thanks @nreese !

nreese added 14 commits December 4, 2018 10:59

super date picker component

b8bc8ca

quick select

fcf1d8f

commonly used ranges

f136b84

recently used

d42a1b1

cleanup quick_select_popover

325168f

refresh interval

0aa79ef

show date range

f0c9544

relative date parsing

e823450

clean up date_popover

c8f3339

relative

08fad42

absolute tab

4ee30fc

implement step forward and step backward

9734df4

add data-test-subjs needed for existing kibana functional tests

4c3e3b6

remove global date picker pattern, add comments about EuiSuperDatePic…

78583b6

…ker props

nreese requested review from cchaos and chandlerprall December 6, 2018 21:10

change log

c2fd0e8

nreese added 3 commits December 6, 2018 14:15

merge with master

412aa69

remove kibana file header

3e69d69

merge with master

f93e5d0

nreese mentioned this pull request Dec 7, 2018

Convert timepicker to EUI elastic/kibana#22433

Closed

5 tasks

only show pretty duration for commonly used ranges and relative to no…

48294f5

…w, now to now isInvalid, recenlty selected in reverse order

cchaos suggested changes Dec 7, 2018

View reviewed changes

cchaos reviewed Dec 7, 2018

View reviewed changes

src/components/date_picker/super_date_picker/pretty_duration.js Show resolved Hide resolved

nreese added 3 commits December 7, 2018 11:13

use full time unit name in pretty duration

ebd5392

pass roundUp to formatTimeString to ensure date button display is acc…

3a4aac0

…urate, make onRefreshChange prop optional

index files for super_date_picker and prefix subcomponents with 'Eui'

db09dc2

nreese added 2 commits December 10, 2018 12:17

make pretty duration clickable, add paragraph to example

b4a0f21

show tooltip when time changes

3da620a

allow setting relative unit when value is zero

0a0c770

cchaos approved these changes Dec 11, 2018

View reviewed changes

snide approved these changes Dec 11, 2018

View reviewed changes

chandlerprall suggested changes Dec 12, 2018

View reviewed changes

nreese added 4 commits December 13, 2018 08:03

merge with master

c4444cc

updates from chandlerprall review

7f74ef7

remove moment-timezone dependency, not needed

759ecda

remove getDerivedStateFromProps from EuiSuperDatePicker, EuiAbsoluteT…

b6bfe05

…ab, EuiRefreshInterval, and EuiRelativeTab

nreese added 4 commits December 14, 2018 14:32

Merge branch 'master' of github.com:elastic/eui into super_date_picker

d422196

move changelog message under master section

97bc872

moved prop types to Wrapped component so they are usable in docs

df7fc4f

check component is mounted for show and hide tooltip

88236cb

nreese mentioned this pull request Dec 14, 2018

move moment to peerDependency in elastic-datemath elastic/kibana#27264

Merged

7 tasks

update to datemath dependency with moment as peer

95100c9

XavierM mentioned this pull request Dec 17, 2018

Add date range picker above kql elastic/kibana#27281

Merged

4 tasks

chandlerprall approved these changes Dec 17, 2018

View reviewed changes

nreese merged commit da70b5c into elastic:master Dec 17, 2018

chrisronline mentioned this pull request Jan 11, 2019

[Monitoring] Refactor monitoring top nav elastic/kibana#28610

Open

chrisronline mentioned this pull request Jan 22, 2019

[Monitoring] Ensure we're prepared for dark mode elastic/kibana#29100

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EuiSuperDatePicker #1351

EuiSuperDatePicker #1351

nreese commented Dec 6, 2018 •

edited by cchaos

Loading

cchaos commented Dec 6, 2018

cchaos commented Dec 7, 2018

cchaos commented Dec 7, 2018

cchaos left a comment

cchaos Dec 7, 2018

cchaos Dec 7, 2018

nreese commented Dec 10, 2018

cchaos commented Dec 10, 2018

cchaos left a comment

snide left a comment

snide commented Dec 12, 2018 •

edited

Loading

nreese commented Dec 12, 2018

snide commented Dec 12, 2018

chandlerprall Dec 12, 2018

nreese Dec 13, 2018

chandlerprall Dec 12, 2018

nreese Dec 13, 2018

nreese commented Dec 14, 2018

nreese commented Dec 17, 2018

XavierM commented Dec 17, 2018 •

edited

Loading

nreese commented Dec 17, 2018

chandlerprall left a comment

EuiSuperDatePicker #1351

EuiSuperDatePicker #1351

Conversation

nreese commented Dec 6, 2018 • edited by cchaos Loading

Summary

Checklist

cchaos commented Dec 6, 2018

cchaos commented Dec 7, 2018

Visual check

cchaos commented Dec 7, 2018

cchaos left a comment

Choose a reason for hiding this comment

cchaos Dec 7, 2018

Choose a reason for hiding this comment

cchaos Dec 7, 2018

Choose a reason for hiding this comment

nreese commented Dec 10, 2018

cchaos commented Dec 10, 2018

cchaos left a comment

Choose a reason for hiding this comment

snide left a comment

Choose a reason for hiding this comment

snide commented Dec 12, 2018 • edited Loading

nreese commented Dec 12, 2018

snide commented Dec 12, 2018

chandlerprall Dec 12, 2018

Choose a reason for hiding this comment

nreese Dec 13, 2018

Choose a reason for hiding this comment

chandlerprall Dec 12, 2018

Choose a reason for hiding this comment

nreese Dec 13, 2018

Choose a reason for hiding this comment

nreese commented Dec 14, 2018

nreese commented Dec 17, 2018

XavierM commented Dec 17, 2018 • edited Loading

nreese commented Dec 17, 2018

chandlerprall left a comment

Choose a reason for hiding this comment

nreese commented Dec 6, 2018 •

edited by cchaos

Loading

snide commented Dec 12, 2018 •

edited

Loading

XavierM commented Dec 17, 2018 •

edited

Loading