Add new React Tabs-based component #1305
Conversation
|
@slowbot Good thing we have a checklist! I just looked at the Storybook example in a narrow (aka mobile) window size and the tabs look pretty funky. Wonder what you think we should do. Truncate and add elipsis… ? Make them horizontally scrolling? |
|
✔️ Deploy Preview for clever-edison-cd22c1 ready! 🔨 Explore the source changes: 123e884 🔍 Inspect the deploy log: https://app.netlify.com/sites/clever-edison-cd22c1/deploys/60bab5d91f79ec000874af63 😎 Browse the preview: https://deploy-preview-1305--clever-edison-cd22c1.netlify.app |
There was a problem hiding this comment.
@jaredcwhite Added a couple of small style updates. Otherwise LGTM
|
@software-project what do you think about Jared's comment concerning the tabs on mobile? |
| afterEach(cleanup) | ||
|
|
||
| describe("<Tabs>", () => { | ||
| it("displays the right CSS", () => { |
There was a problem hiding this comment.
This is just testing implementation details - is there a way we can test component or prop functionality without looking at class names?
There was a problem hiding this comment.
I tried to at least test the left/right arrow keys like on TabNav but was unsuccessful. Spent a while trying to get the events to fire but something was off. Basically we'd be testing a third-party component anyway, so not sure how much we need to dig into that. Maybe write a test to simulate clicking on the different tabs and double-check if that works?
There was a problem hiding this comment.
Fair point on the 3rd party component :) I'll take a quick peek
There was a problem hiding this comment.
Looks like it's really well tested on their end. I took a peek just to see how they did it bc I also couldn't get the tabs to change, and it looks like they're pulling in another RTL sub-package called userEvent (🤷♀️) I would still recommend we change the first test though. I don't think we should rely on any of their class names, as we could pull a new version of this and it could break the test but no functionality. Maybe we can just test our own prop? Something like:
it("can pass a custom class name", () => { const { container } = render( <Tabs defaultIndex={1}> <TabList> <Tab className="other-tab">Other</Tab> <Tab>Default</Tab> </TabList> <TabPanel className="other-panel">OtherPanel</TabPanel> <TabPanel>DefaultPanel</TabPanel> </Tabs> ) expect(container.getElementsByClassName("other-tab").length).toBe(1) expect(container.getElementsByClassName("other-panel").length).toBe(1) })
There was a problem hiding this comment.
Thanks @emilyjablonski, I'll try that…good call on not hardcoding their internal class names. I'll see if I can remove one from our stylesheet in lieu of a custom one as well.
|
@jaredcwhite There are a few options for mobile tabs but I think stacking makes the most sense |
|
@slowbot That sounds good. I'll work on a stacked presentation for mobile. |
|
One comment on the test, otherwise LGTM! I personally think the mobile piece could be a separate issue if you want to break it out. Up to you. |
|
I got the mobile tab stacking implemented and improved the resiliency of the tab/panel CSS and testing. |
* Add new React Tabs-based component, switch old Tab to TabNavItem * Add disabled tab style * adding slight style updates * Responsive tab design * Update tabs selected class name, tests * Add info on Tabs component to Changlog Co-authored-by: Jesse Arnold <jessearnold@Jesses-MacBook-Pro.local>
This reverts commit 87c1987.
This reverts commit 87c1987.
Pull Request Template
Issue
Addresses #1157
Description
This PR adds the React Tabs dependency and wraps those components so we get default styling. The previous
Tabsubcomponent ofTabNavhas been renamed toTabNavItem.Type of change
How Can This Be Tested/Reviewed?
Checklist: