Conversation
Co-Authors @BinaryMuse
|
This pull request is being automatically deployed with ZEIT Now (learn more). 🔍 Inspect: https://zeit.co/primer/primer-components/oc9137i72 |
Co-Authored-By: Michelle Tilley <binarymuse@github.com>
Co-Authored-By: Michelle Tilley <binarymuse@github.com>
colebemis
left a comment
There was a problem hiding this comment.
Holy cow. Nice work, @emplums! 🎉 I left a few minor comments but nothing blocking.
I have a couple visual notes as well:
-
Using the
Filtercomponent above the tabs looks a little weird because filter has a white background. It should probably have a gray background to match.

-
I noticed that navigating between tabs using the keyboard is a little different in this SelectMenu vs the one on dotcom. On dotcom, you navigate between tabs using the left and right arrow keys. In this component, you navigate between tabs using the
tabkey. I don't know if that's an issue but thought it was worth pointing out.
|
This is awesome! Found this PR when you mentioned me on Twitter and just popped by for a look, it's always fun seeing other Select implementations 😀 Really nice work 👏 |
|
Thanks @JedWatson!! 🙏 |
BinaryMuse
left a comment
There was a problem hiding this comment.
No further feedback from me! This looks great ✨
src/__tests__/SelectMenu.js
Outdated
| }) | ||
|
|
||
| // clicking on an item closes the modal | ||
| // |
SelectMenu components! This PR contains a wrapper
SelectMenuand ✨6 ✨ subcomponents to build out our complex SelectMenu :) I've also added
ButtonTableListSelect Menu Components
SelectMenu
initialTabprop and sets that tab to be open initiallyselectedTabstateopenstate for menu contentScreenshot 📸
SelectMenu.Modal
roleattribute for a11y on wrapper componentrole="menu"with auland provides list stylingtitleprop to add a heading for the select menu contentScreenshot 📸
SelectMenu.Filter
Screenshot 📸
SelectMenu.Tabs
role="tablist"for a11yScreenshot 📸
SelectMenu.Tab
initialTabas selectedselectedTabcontext when a tab is clickedonClickSelectMenu.Item
selectedproparia-checked="true"to selected itemsScreenshot 📸
SelectMenu.Divider
Screenshot 📸
SelectMenu.Footer
Screenshot 📸
Other Components
ButtonTableList (this needs a new name 😂 )
TO DO:
SelectMenu.Filteris offNote: This is marked as a major release change because React recommends marking a change as breaking when introducing forwardRef: https://reactjs.org/docs/forwarding-refs.html#note-for-component-library-maintainers