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] relative tab, timestamp: Use roundUp, switch pos with "Round To" btn #1827

Merged
merged 6 commits into from
Apr 26, 2019

Conversation

kertal
Copy link
Member

@kertal kertal commented Apr 12, 2019

Summary

So when you set e.g. 70 years ago in the To field of the date
range with "Round to the year" active, the To field's value is rounded
up. So instead of:

Jan 1, 1949 @ 00:00:00.000

It's

Jan 1, 1949 @ 23:59:59.999

Since now this date is less then 70 years in the past, moment.js fromNow
displays 69 years

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

@chandlerprall
Copy link
Contributor

The text is displayed as _~ 69 years ago_ which is accurate, and better reflects the value after the Round to year modifier is applied. I think it would be disingenuous to users if we modify the display value.

@nreese any thoughts here?

@chandlerprall
Copy link
Contributor

@cchaos your thoughts as well

@kertal
Copy link
Member Author

kertal commented Apr 12, 2019

Maybe it would be clearer if the rounding of the displayed date in the popover would equal the rounding of the To button's string
Elastic_UI_Framework_-_Date_Picker

there was some user's confusion that led to this PR, here's the original issue in the kibana repo:
elastic/kibana#33432

@cchaos
Copy link
Contributor

cchaos commented Apr 12, 2019

Making them the same I think it the most obvious step here. Also, I'd mov the rounding switch above the calculated date.

@nreese
Copy link
Contributor

nreese commented Apr 12, 2019

Rounding the to string is required. One example of where this is necessary is for date math strings. The quick range today is from: now/d and to: now/d. Without the round up on the to string, they are both the same time values.

https://github.com/elastic/kibana/blob/master/src/legacy/core_plugins/kibana/ui_setting_defaults.js#L797

kertal added 3 commits April 24, 2019 11:38
So when you set e.g. 70 years ago in the To field of the date
range with "Round to the year" active, the To field's value is rounded
up. So instead of:

Jan 1, 1949 @ 00:00:00.000

It's

Jan 1, 1949 @ 23:59:59.999

Since now this date is less then 70 years in the past, moment.js fromNow
displays 69 years
- to sync displayed value with the value displayed in the
popover button
@kertal kertal force-pushed the pr-1826-eui-super-date-picker branch from a09853e to d2b2f9e Compare April 24, 2019 09:40
@kertal kertal changed the title Remove roundUp flag from formatTimeString function Use roundUp flag in displayed timestamp of the relative tab Apr 24, 2019
@kertal
Copy link
Member Author

kertal commented Apr 24, 2019

So i've adapted the displayed timestamp in the Relative Tab. With the usage of the roundup prop it should be correct:
Elastic_UI_Framework_-_Date_Picker

@kertal
Copy link
Member Author

kertal commented Apr 24, 2019

@cchaos that's the way it would look like when rounding switch would be rendered above calculated date.

Bildschirmfoto 2019-04-24 um 12 10 16

@cchaos
Copy link
Contributor

cchaos commented Apr 24, 2019

I'll defer to @nreese and @chandlerprall on the code side, but yes, I think it's worthwhile to move the switch to above the rendered(read only) date/time input. That way in both the absolute and relative, the rendered date/time is always at the bottom.

However it looks like the spacing is a bit off in your screenshot. If you push the code, I can tell you how to fix the spacing.

@@ -98,6 +95,9 @@ export class EuiRelativeTab extends Component {
onChange={this.onRoundChange}
/>
</EuiFormRow>
<EuiFormRow>
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you remove the EuiFormRow's from wrapping the readonly text and the switch? They're not providing anything but messing up some spacing. Then you can just some spacers like so:

<EuiSpacer size="s" />

<EuiSwitch ... />

<EuiSpacer size="m" />

<EuiFieldText ... />

Which should end up looking like this:

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

LGTM, waiting for engineer to approve. You'll also need to add a CHANGELOG.md entry (just follow the pattern that is there and use past tense).

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.

lgtm
code review, tested changes in chrome

@kertal
Copy link
Member Author

kertal commented Apr 25, 2019

retest

@kertal kertal changed the title Use roundUp flag in displayed timestamp of the relative tab [EuiSuperDatePicker] relative tab, timestamp: Use roundUp, switch pos with "Round To" btn Apr 25, 2019
@kertal
Copy link
Member Author

kertal commented Apr 25, 2019

so i've added changelog, went over the checklist, so we're ready to merge?

@kertal kertal merged commit 87f944c into elastic:master Apr 26, 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.

4 participants