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

WIP Convert NavDrawer to TypeScript #2772

Closed
wants to merge 4 commits into from

Conversation

dannyfritz
Copy link

@dannyfritz dannyfritz commented Jan 17, 2020

Regarding #2658

Converting NavDrawer to TypeScript.

@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?

@dannyfritz dannyfritz changed the title WIP Convert NavDrawer to TypeScript #2658 WIP Convert NavDrawer to TypeScript Jan 17, 2020
@dannyfritz
Copy link
Author

This is a WIP. I'm at a point where I could use some help and eyes. There are some choices I made that I'm unsure are correct. And then there a few choices left to be made that I wasn't sure how to choose.

@chandlerprall chandlerprall self-assigned this Jan 21, 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.

Took an initial pass, didn't see anything major. Most comments are around EUI's typescript component patterns.

Let's address the any and unknown types once things are a bit cleaner / more final. Looks like it will be solved with a handful of generics.

src/components/nav_drawer/nav_drawer.tsx Outdated Show resolved Hide resolved
src/components/nav_drawer/nav_drawer.tsx Outdated Show resolved Hide resolved
src/components/nav_drawer/nav_drawer.tsx Outdated Show resolved Hide resolved
src/components/nav_drawer/nav_drawer.tsx Outdated Show resolved Hide resolved
src/components/nav_drawer/nav_drawer.tsx Outdated Show resolved Hide resolved
src/components/nav_drawer/nav_drawer.tsx Show resolved Hide resolved
src/components/nav_drawer/nav_drawer.tsx Outdated Show resolved Hide resolved
src/components/nav_drawer/nav_drawer_flyout.tsx Outdated Show resolved Hide resolved
src/components/nav_drawer/nav_drawer_group.tsx Outdated Show resolved Hide resolved
@dannyfritz
Copy link
Author

dannyfritz commented Feb 10, 2020

There are still about 5 locations of TypeScript errors.

Thanks for the detailed feedback. Tried to address all of it. Please mark as resolved any comments you feel were adequately addressed.

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.

Thanks for the updates!

I've added one new request, and closed the requests that I saw in effect, though there are a handful of unresolved requests/comments still pending.

src/services/throttle.ts Show resolved Hide resolved
@chandlerprall
Copy link
Contributor

@dannyfritz Know you're still busy, just checking in :)

@cla-checker-service
Copy link

cla-checker-service bot commented Mar 1, 2020

❌ Author of the following commits did not sign a Contributor Agreement:
9852ebf, c79477c, cc9ee52, 081c41a

Please, read and sign the above mentioned agreement if you want to contribute to this project

@dannyfritz
Copy link
Author

There are still 2 errors left, but I think someone should take those 2 over the finish line for me. They are either quirks in how the library is using HTML elements, or the HTML elements assigned to the component is too specific.

@dannyfritz dannyfritz force-pushed the 2658-nav-drawer-ts branch from 72e5332 to 081c41a Compare March 2, 2020 00:09
@chandlerprall
Copy link
Contributor

Thanks @dannyfritz ! We can address the remaining issues. Could sign our Contributor License Agreement at https://www.elastic.co/contributor-agreement using the email address associated with your GitHub account?

@ronnyek
Copy link

ronnyek commented Mar 10, 2020

I too ran into this very problem today (that its not exposed in eui.d.ts.)

@chandlerprall
Copy link
Contributor

@dimitropoulos any interest in taking the rest of this one on? No pressure to, but if you're still looking for things to typescript-ify... :)

@dimitropoulos
Copy link
Contributor

dimitropoulos commented Mar 27, 2020

I'd be more than happy to help out! My 🔝 priority is #2891 right now because it's so big (and so close!), but after that's done I can make my way here next!

That said, I have two other components I'm working on for material-ui, so I haven't quite figured out how I'm gonna prioritize which ones I work on (especially with my nights-and-weekends time being so restricted due to the recent global events). By all means, if someone has time to get to it first, I won't be offended. It'd likely (comparing to the other ones I've done so far) be a one-night affair (or so), so no worries about toe-stepping!

😸

@anishagg17
Copy link
Contributor

@chandlerprall can I work upon this?

@dimitropoulos
Copy link
Contributor

I (mostly) finished it this weekend but haven't had a chance to push yet - maybe gimme until tonight @anishagg17 and if I (for whatever reason) can't get the PR up, then please feel free!

@dimitropoulos
Copy link
Contributor

@anishagg17: I pushed it #3268

(although I wrote the code a few days ago, I wanted to notate my changes (which takes some time) so that's why I hadn't pushed it until now - thanks for your patience everyone!)

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.

8 participants