Adds UnderlinePanels to drafts/
#4550
Conversation
stylistic tweaks more stylistic tweaks
🦋 Changeset detectedLatest commit: 0f0b58c The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Added @keithamus as a reviewer since he's familiar with @github/tab-container-element, which does a lot of the heavy-lifting here. |
size-limit report 📦
|
| @@ -7,7 +7,7 @@ | |||
| "importPath": "@primer/react", | |||
| "props": [ | |||
| { | |||
| "name": "afterSelect", | |||
| "name": "onSelect", | |||
There was a problem hiding this comment.
Ahh must have forgotten to update this, thank you!
|
|
||
| Adds UnderlinePanels component. It's like UnderlineNav, but for rendering semantic tabs instead of links. | ||
|
|
||
| <!-- Changed components: UnderilnePanels --> |
There was a problem hiding this comment.
I don't think we need the specify the changed components anymore since Primer notifications project is abandoned.
| import useLayoutEffect from '../utils/useIsomorphicLayoutEffect' | ||
| import {defaultSxProp} from '../utils/defaultSxProp' | ||
| import Link from '../Link' | ||
| import {UnderlineTab} from '../internal/components/UnderlineTabbedInterface' |
There was a problem hiding this comment.
How would you feel about renaming this to UnderlineItem? It feels a bit misleading to me have the "tab" in underlinenav.
There was a problem hiding this comment.
I think UnderlineTab might be clearer since the children behave as role=tab and and role=tabpanel
There was a problem hiding this comment.
I am confused. I think this is shared between UnderlineNav and UnderlinePanel, isn't it? And in this file, UnderlineTab doesn't have a role tab or tabpanel - let me know if I am missing something!
There was a problem hiding this comment.
Sorry, I misunderstood your comment! UnderlineItem works great.
| "name": "UnderlinePanels", | ||
| "status": "draft", | ||
| "a11yReviewed": false, | ||
| "stories": [ |
There was a problem hiding this comment.
I think we can still keep these story ids but we don't actually need to specify. The build script pulls out feature stories by default
packages/react/src/drafts/UnderlinePanels/UnderlinePanels.docs.json
Outdated
Show resolved
Hide resolved
| ${sx}; | ||
| ` | ||
|
|
||
| export const StyledUnderlineTabList = styled.ul` |
There was a problem hiding this comment.
Similar naming suggestion: to make it more generic, how would you feel about renaming this to StyledUnderlineList?
I couldn't reproduce it either 😞 |
Co-authored-by: Armağan <broccolinisoup@github.com>
There was a problem hiding this comment.
In my initial review, I only focused on the UnderlineNav but this time went over the changes of UnderlinePanel. I left some comments/questions! Looking forward to hearing what you think 🙌🏻
Also I think we should run integration tests at dotcom mainly to make sure we didn't break anything in UnderlineNav
| /** | ||
| * Whether this is the selected tab | ||
| */ | ||
| 'aria-selected'?: boolean |
There was a problem hiding this comment.
I wonder if we should also add aria-controls attribute to each tab and associate the values to their respective panels. (based on APG Tab Pattern) but also there is not enough support for aria-controls 🤔 What are your thoughts?
There was a problem hiding this comment.
We don't use aria-controls for other tabs, so I wouldn't introduce it here. For example, we don't attempt setting aria-controls in @github/tab-container-element
|
|
||
| export const WithIcons = () => ( | ||
| <UnderlinePanels aria-label="Tabs with icons"> | ||
| <UnderlinePanels.Tab icon={CodeIcon}>Code</UnderlinePanels.Tab> |
There was a problem hiding this comment.
This is nitpick but how about using tab panel examples from dotcom or just tab1, tab2 etc? Because Code, Issues etc are navigational items and I feel we might mislead folks by using our core navigational usages for tab panels examples. Let me know what you think!
There was a problem hiding this comment.
Great call. Will update.
| ) | ||
|
|
||
| export const WithIconsHiddenOnNarrowScreen = () => ( | ||
| <UnderlinePanels aria-label="Tabs with icons"> |
There was a problem hiding this comment.
I experience flickering in this story and ot might not be specific to only this story though but not sure why it happens. When I resize a scroll bar (I think?) appears for a hot second and then drops the icons. Sharing a screen recording, let me know what you think
Video preview describes the behaviour I mentioned just above
Rewatch.Screen.Recording.-.2024-05-21.at.4.19.mp4
There was a problem hiding this comment.
I'm not sure how we'd get around this and keep the tablist scrollable. This can be something we tackle when/if we introduce the same overflow behavior we have for UnderilneNav.
I wouldn't consider this a blocker, but would you or other reviewers?
| const defaultId = useId() | ||
| const parentId = props.id ?? defaultId |
There was a problem hiding this comment.
I think we can give the custom id as a param to the hook like const parentId = useId(props.id) to make it use the custom one. I find it cleaner that way but totally up to you 😊
There was a problem hiding this comment.
Oh yea I forgot we could do that. Will update.
|
Reviewers: I'll be out for 2 weeks. Please merge if this gets approved before I'm back. |
|
Snapshots need to be updated again since we changed text in stories. |
Relates to https://github.com/github/primer/issues/3108
This component is visually identical to
UnderlineNav. The biggest differences are:Kapture.2024-05-01.at.16.34.03.mp4
Changelog
New
Adds
UnderlinePanelscomponentChanged
Refactors
UnderlineNavto share component styles withUnderlinePanelsRemoved
Nothing removed
Rollout strategy
Testing & Reviewing
This should more or less behave like PVC's UnderlinePanels
Merge checklist