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.

Sign up for GitHub

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

Jump to bottom

Converts NavDrawer, NavDrawerGroup, and NavDrawerFlyout to TypeScript #3268

Merged
merged 7 commits into from
Apr 13, 2020
Merged

Converts NavDrawer, NavDrawerGroup, and NavDrawerFlyout to TypeScript #3268

merged 7 commits into from
Apr 13, 2020

Conversation

dimitropoulos
Copy link
Contributor

@dimitropoulos dimitropoulos commented Apr 7, 2020 •
edited by thompsongl
Loading

Summary

closes #2658
closes #2772

I decided it would be most scientific (and interesting!) to do a clean-room implementation (at first, anyway) from the original PR #2772. Upon taking a look, a lot of the things are the same, but many are different.

As always, I tried very hard to stick to the components in question to avoid scope creep ("Rome wasn't built in a day"-kinda thing), as well as notating every single instance where I feel that there's potential for logical changes :)

Checklist

  • Check against all themes for compatibility in both light and dark modes
  • Checked in mobile
  • Checked in IE11 and Firefox
  • Props have proper autodocs (could use a double check on this!)
  • Added documentation examples
  • Added or updated jest tests
  • Checked for breaking changes and labeled appropriately
  • Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

@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
dimitropoulos commented Apr 7, 2020
View reviewed changes
src/components/list_group/list_group_item.tsx Show resolved Hide resolved
src/components/nav_drawer/nav_drawer.tsx Show resolved Hide resolved
src/components/nav_drawer/nav_drawer.tsx Show resolved Hide resolved
src/components/nav_drawer/nav_drawer.tsx Show resolved Hide resolved
src/components/nav_drawer/nav_drawer.tsx Show resolved Hide resolved
src/components/nav_drawer/nav_drawer_flyout.tsx Show resolved Hide resolved
src/components/nav_drawer/nav_drawer_flyout.tsx Show resolved Hide resolved
src/components/nav_drawer/nav_drawer_flyout.tsx Show resolved Hide resolved
src/components/nav_drawer/nav_drawer_group.tsx Show resolved Hide resolved
src/components/nav_drawer/nav_drawer_group.tsx Show resolved Hide resolved
@dimitropoulos dimitropoulos mentioned this pull request Apr 7, 2020
Closed
@thompsongl thompsongl mentioned this pull request Apr 9, 2020
Closed
@thompsongl
Copy link
Contributor

jenkins test this

@kibanamachine
Copy link

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

thompsongl
thompsongl suggested changes Apr 9, 2020
View reviewed changes
Copy link
Contributor

@thompsongl thompsongl left a comment

Choose a reason for hiding this comment

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

Thank you, @dimitropoulos!

Nice work, as always, so I think this is close.

src/components/nav_drawer/nav_drawer.tsx Outdated Show resolved Hide resolved
src/components/nav_drawer/nav_drawer_group.tsx Outdated Show resolved Hide resolved
@dimitropoulos
Copy link
Contributor Author

thanks for the swift review @thompsongl! I think it's pretty much ready to go now 🐎

thompsongl
thompsongl suggested changes Apr 13, 2020
View reviewed changes
Copy link
Contributor

@thompsongl thompsongl left a comment

Choose a reason for hiding this comment

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

Just a couple small things and the merge conflict to fix.

Confirmed that these changes work with very minor upates to Kibana

src/components/nav_drawer/nav_drawer.tsx Outdated Show resolved Hide resolved
src/components/nav_drawer/nav_drawer.tsx Outdated Show resolved Hide resolved
dimitropoulos and others added 3 commits April 13, 2020 13:42
@dimitropoulos @thompsongl
Update src/components/nav_drawer/nav_drawer.tsx
d04bc4d 
Co-Authored-By: Greg Thompson <thompson.glowe@gmail.com>
@dimitropoulos @thompsongl
Update src/components/nav_drawer/nav_drawer.tsx
419cc40 
Co-Authored-By: Greg Thompson <thompson.glowe@gmail.com>
@thompsongl
Merge branch 'master' into navdrawer-typescript
f0d9c92
@thompsongl
Copy link
Contributor

jenkins test this

@kibanamachine
Copy link

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

thompsongl
thompsongl approved these changes Apr 13, 2020
View reviewed changes
Copy link
Contributor

@thompsongl thompsongl left a comment

Choose a reason for hiding this comment

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

LGTM!

Built locally and tested the output in Kibana (type check script and manual smoke test)

@thompsongl thompsongl merged commit a83d879 into elastic:master Apr 13, 2020
@tushar22-tg
Copy link

@dimitropoulos are you a GSoC applicant?

@dimitropoulos
Copy link
Contributor Author

@dimitropoulos are you a GSoC applicant?

@tushar22-tg Google Summer of Code? no, just a friendly neighborhood open source contributor :)

@dimitropoulos dimitropoulos deleted the navdrawer-typescript branch April 13, 2020 20:08
@thompsongl thompsongl mentioned this pull request May 8, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thompsongl thompsongl thompsongl approved these changes

Assignees
No one assigned
Labels
typescript
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

EuiNavDrawer needs to be converted to TS
5 participants
@dimitropoulos @kibanamachine @thompsongl @tushar22-tg @snide