Adds NavList.GroupHeading component #5106
Conversation
🦋 Changeset detectedLatest commit: 5765a19 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 |
|
👋 Hi, this pull request contains changes to the source code that github/github depends on. If you are GitHub staff, we recommend testing these changes with github/github using the integration workflow. Thanks! |
github-actions
bot
added
the
integration-tests: recommended
mperrotti
temporarily deployed
to
github-pages
GitHub Actions
Inactive
size-limit report 📦
|
mperrotti
temporarily deployed
to
github-pages
GitHub Actions
Inactive
mperrotti
temporarily deployed
to
github-pages
GitHub Actions
Inactive
There was a problem hiding this comment.
The NavList group headings would be styled exactly the same except when they're hovered. The first group heading in this image is being hovered.
I'm seeing some different styles in the heading between the "With Group" and "With Group Heading Links" stories (mainly the color), wondering if that's intended :
Also wondering what would happen if a user used this in alongside the title prop (and if we even care to cover/prevent for that use case)
| const GroupHeading: React.FC<NavListGroupHeadingProps> = ({sx: sxProp = defaultSxProp, ...rest}) => { | ||
| return ( | ||
| <ActionList.GroupHeading | ||
| as="h3" |
There was a problem hiding this comment.
(nit) optional suggestion:
from line 285:
TODO: API update to give flexibility to NavList.Group title's heading level
Maybe this is a good opportunity to do that here? (take as as a prop that defaults to h3 and can be overriden if necessary). We could then use this same component for the title prop instead of ActionList.GroupHeading as well 🤔
Wouldn't make the title's prop heading level itself configurable, but might offer an alternative manually through the new GroupHeading
This is an intentional style change we decided to make last week.
Great question! I'll look into this and add some guards to prevent undesired results. |
| sx={merge<SxProp['sx']>( | ||
| { | ||
| '> a {': { | ||
| color: 'var(--fgColor-default)', | ||
| textDecoration: 'inherit', | ||
| ':hover': {textDecoration: 'underline'}, | ||
| }, | ||
| }, | ||
| sxProp, | ||
| )} |
There was a problem hiding this comment.
Do we still want to use sx here?
There was a problem hiding this comment.
Is it possible to use regular CSS for component styling in PRC yet?
There was a problem hiding this comment.
If you're adding new styles I think it's safe to go directly to CSS modules. The risk is entirely around the fact that there could be other styles in dotcom relying on the existing sx styling architecture.
keithamus
left a comment
keithamus
left a comment
There was a problem hiding this comment.
Overall LGTM, but we should consider if we want to use sx or not.
mperrotti
temporarily deployed
to
github-pages
GitHub Actions
Inactive
We needed linked group headings when developing the new Primer docs. Instead of faking it by using
ActionList.GroupHeading, we discussed updating theNavListAPI to allow renderingNavList.Groupheadings usingNavList.GroupHeadinginstead of the more restrictivetitleprop:<NavList.Group title="Group title">This was discussed in a weekly Primer patterns/API working sesh on Sept 24 2024 (notes)
The NavList group headings would be styled exactly the same except when they're hovered. The first group heading in this image is being hovered.
Changelog
Adds
NavList.GroupHeadingcomponentNew
NavList.GroupHeadingcomponentChanged
Removed
Rollout strategy
Testing & Reviewing
Merge checklist