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

[Date Picker] Updates to Year and Month via dropdown should trigger onChange #1817

Merged
merged 7 commits into from
Apr 12, 2019

Conversation

thompsongl
Copy link
Contributor

@thompsongl thompsongl commented Apr 10, 2019

Summary

Fixes #1806
Co-opts the adjustDateOnChange prop from react-datepicker and defaults to true.

Prop is now exposed in the EUI API, so opting out is possible by passing adjustDateOnChange={false}

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
Copy link
Contributor

cchaos commented Apr 10, 2019

One thing I noticed is that changing the year or month now also changes the date to always be the first. I would expect the date to stay the same as well so changing April 10, 2019 to 2025 would be April 10, 2025 not April 1, 2025:

@chandlerprall
Copy link
Contributor

One thing I noticed is that changing the year or month now also changes the date to always be the first. I would expect the date to stay the same as well so changing April 10, 2019 to 2025 would be April 10, 2025 not April 1, 2025:

My intuition is to agree with this, however I'm not sure what we'd want to do when the new target year/month doesn't include the selected day, e.g. switching from January 31 to February, should it select the 28th/29th ?

@thompsongl
Copy link
Contributor Author

thompsongl commented Apr 11, 2019

Good catch, @cchaos
What's been our policy on modifying EUI's package/react-datepicker? The modifications to needed to make the date persist would likely need to happen in that lib. Good news, though, @chandlerprall's concern looks to be taken care of by react-datepicker's date math; Feb. 29, 2020 (leap year) adjusted to 2019 results in Feb. 28, 2019 without error. The last day of the month discrepancy seems to be to only place a date mismatch can occur, and it'd be accounted for (assuming that Mar. 1, 2019 doesn't make more sense).

@cchaos
Copy link
Contributor

cchaos commented Apr 11, 2019

however I'm not sure what we'd want to do when the new target year/month doesn't include the selected day, e.g. switching from January 31 to February, should it select the 28th/29th ?

So we're good when it comes to that situation based on your comments @thompsongl ?

@thompsongl
Copy link
Contributor Author

thompsongl commented Apr 11, 2019

So we're good when it comes to that situation based on your comments

Yes. Jan. 31, 2019 and Mar. 31, 2019 would both adjust to Feb. 28, 2019. It would follow the same pattern in place today when using the arrow buttons to navigate months.

@snide
Copy link
Contributor

snide commented Apr 11, 2019

What's been our policy on modifying EUI's package/react-datepicker? The modifications to needed to make the date persist would likely need to happen in that lib.

Our policy at this point is that it's a fork we're independently maintaining for our own needs, so it's OK to change the installed package lib (we've done this already). We took a stab at some bigger upstream stuff, but didn't get the best response.

Long term I think we'd like to build one of these internally from scratch, but the priority isn't there atm.

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

Changes LGTM

Copy link
Contributor

@snide snide left a comment

Choose a reason for hiding this comment

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

Did a functional test of this one locally. It responds how we discussed in the comments. Nice work!

@thompsongl thompsongl merged commit f277059 into elastic:master Apr 12, 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.

Datepicker doesn't let you update the year without updating date/time
4 participants