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

converts all DatePicker family of components to TypeScript #2891

Merged
merged 11 commits into from
Apr 13, 2020

Conversation

dimitropoulos
Copy link
Contributor

closes: #2884

I didn't realize there were so many nested components when I made the original ticket, haha, but I was on a roll and I think I got the vast majority of issues handled.

This is still a draft PR: I still need to do a once-over, but it wouldn't be bad if anyone wanted to take a look.

@kibanamachine
Copy link

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@dimitropoulos
Copy link
Contributor Author

happy leap day!

So I wanted to update some things I've been running into regarding the react-datepicker dependency. So I did some archaeology and found some context for what got things to where they are now with react-datepicker being vendored directly in: #1607, #1618, #2005, and a few others.

I think I have a decent idea of what's going on, although I must admit I'm not clear on why it's going on. All the same, I was running into some problems, so I tried to use upstream directly (and remove the vendoring) - just to see what would happen. Well dimitropoulos@069554c is what happened.

TLDR; There have been some very significant changes in react-datepicker, namely the removal of moment, which has some deep cutting problems like the removal of the utcOffset which effectively translates to the loss of timezone support (!!!?!?!), something the react-datepicker authors seemed to be aware of. I was hoping that a drop in replacement (maybe.. hahah.. just maybe...), but it definitely won't be - so I'm jettisoning that approach. I get very nervous about making logical changes in TypeScript conversions because it can be a never-ending rabbit hole of ever-expanding scope creep if you don't stay focused on the TypeScript-mandatory changes.

I'll keep plugging along. I don't think I'm terribly far off - but it'd be nice to have some general background on what the situation is (and why), as well as what the gameplan is for the future with react-datepicker.

@chandlerprall
Copy link
Contributor

chandlerprall commented Mar 1, 2020

the vendored packages/react-datepicker.js code

react-datepicker pseudo-fork (we added a lot of keyboard & screen reader accessibility, in addition to some style changes) is in https://github.com/elastic/eui/tree/master/packages/react-datepicker/

@dimitropoulos
Copy link
Contributor Author

I made some great progress tonight. Everything is in TypeScript, all the tests pass, and a smoke test yielded no smoke.

As always with a PR like this, I really want to go over things with a fine tooth comb before calling it ready for review and highlight any and all areas where I think there's even slight potential for logical change. I tried, again, to make as few logical changes as physically possible, but there were a few loopdy-doos with the react-datepicker props that I want to go back and line up one-by-one. I aspire to do this tomorrow night or this weekend so that hopefully by Monday this will be ready to review and can get out the door this coming week.

Copy link
Contributor Author

@dimitropoulos dimitropoulos left a comment

Choose a reason for hiding this comment

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

okie dokie! Should be ready to review!

@dimitropoulos dimitropoulos marked this pull request as ready for review March 10, 2020 01:19
@chandlerprall chandlerprall self-requested a review March 11, 2020 19:26
@chandlerprall chandlerprall self-assigned this Mar 11, 2020
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.

Halfway(ish) through. Feel free to ignore until I finish up tomorrow

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.

Phew! Thanks for this one

Mentions of prop x was previously optional/required are more of an observation; I'm resistant to changing these by default, but if these fix bugs, etc, I'm not against the changes

@dimitropoulos
Copy link
Contributor Author

just to say it out loud, I'm really trying hard to scratch out some time to finish this up (it's so close!!), but schools/childcare closing in addition to all the other sudden life changes with the pandemic has meant a drastic drastic reduction in my nights+weekends time (when I can do work like this PR). as with anything, I have a very low ego when it comes to my work so I wouldn't be offended at all if you see it best to take it over. I get nervous about such a big PR that touches so many files sitting around for long..

I really do expect to be able to finish it up in a week or so, but part of me thinks that naive, haha, so I wanted to let you know where the work currently stands in my queue. I was really hoping to have time to do it tonight but it didn't pan out again..

@chandlerprall
Copy link
Contributor

@dimitropoulos thanks for the updates, and absolutely no worries! We'll see who gets to it first :)

@dimitropoulos
Copy link
Contributor Author

progress!

There's a bit more to do here, but it's getting there. It took more time than I had hoped to rebase before starting, so I ran out of time to grab all the current unresolved comments, but it's getting closer all the time. I'll continue trying my best to find time for this at night. I'd really love to see this one cross the finish line!

My best to everyone at Elastic during this strange and stressful time! It was a real joy to work on this tonight instead of reading the news, haha. So, thanks!

@dimitropoulos
Copy link
Contributor Author

@chandlerprall I'm about 95% through all the comments, but I'm out of time for tonight. I think it'd be a good time to take another look if you can. The merge conflicts for this are going to get harder and harder (in fact, this time around, I've opted to just not try and rebase it). I'm gonna try really really hard to eek out some time to get this wrapped up. It'd be a shame for it to keep stumbling into merge-conflict-hell, haha.

@chandlerprall
Copy link
Contributor

Changes so far look good! I've replied to a couple open comment threads, and will look into the issues around exporting ReactDatePicker / ReactDatePickerProps

@dimitropoulos
Copy link
Contributor Author

ok, we gotta be getting super super close! I rebased it again (which took a bit of effort, but I think I got it right! hopefully, anyway, haha) so at least it's up to date.

The last significant quandry is how to handle the types for ReactDatePicker. If we can get that one figured out, I'd say it's gotta pretty darn close to merge-able.

Lemme know!

@chandlerprall
Copy link
Contributor

@dimitropoulos I pushed 3 commits to your branch, and linked each on their respective conversations. If you're around and can double check, especially those onChange ones, that'd be great 👍

@dimitropoulos
Copy link
Contributor Author

@dimitropoulos I pushed 3 commits to your branch, and linked each on their respective conversations. If you're around and can double check, especially those onChange ones, that'd be great 👍

I can take a look in the next few hours!

@dimitropoulos
Copy link
Contributor Author

dimitropoulos commented Apr 10, 2020

update: dinner went faster than expected because it was, like, wayy wayy scrumptious and I ate really fast, haha - so I'm looking now!

@dimitropoulos
Copy link
Contributor Author

aside from one little thing I just noticed, I think we're (can I say it???)... done!? The little thing, frankly, is just as well suited to be a followup PR. It's not really a typescript thing anyway.

I rebased it just now, so hopefully we're good to go!

@chandlerprall
Copy link
Contributor

jenkins test this

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_2891/

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.

LGTM! Gonna test a build with these changes in Kibana

@chandlerprall
Copy link
Contributor

chandlerprall commented Apr 13, 2020

Pushed regression fixes in a74b42b

  • Export the external shape of EuiDatePickerProps, not the one with defaultProps as required
  • Export OnRefreshChangeProps, OnTimeChangeProps, and OnRefreshProps

Also merged master into this branch to fix the changelog conflict.

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; Pulled & tested and retested and tested more locally :)

Thanks again @dimitropoulos !! It's very appreciated.

@chandlerprall
Copy link
Contributor

jenkins test this

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_2891/

@dimitropoulos
Copy link
Contributor Author

dimitropoulos commented Apr 13, 2020

ship it

@chandlerprall
Copy link
Contributor

Jest OOMed; jenkins test this

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_2891/

@chandlerprall chandlerprall merged commit cddaf28 into elastic:master Apr 13, 2020
@dimitropoulos dimitropoulos deleted the date-picker-ts branch April 13, 2020 23:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

EuiDatePicker needs to be converted to TypeScript
5 participants