-
Notifications
You must be signed in to change notification settings - Fork 143
feat(sideNav): Adds support for grouped nav with headers and dividers. #4754
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.
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
base: main
Are you sure you want to change the base?
Conversation
@dlabaj @wise-king-sullyman @mcoker I took a swing at adding in the new nav design support we need via Cursor. Not sure if it did a great job -- when you get time, it could use more experienced eyes and edits! I was thinking that we could add this support in advance of actually enabling it? That way the only remaining changes on my end will be doc-site updates, which sound less disruptive to astro work. But lmk if you have different thoughts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty awesome @edonehoo! It is creating some invalid markup though. I just glanced at it and left a possible direction to try and fix. Wanna see if cursor can figure that out?
|
||
return ( | ||
<Nav aria-label="Side Nav" theme="light"> | ||
<NavList className="ws-side-nav-list"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just noting that this creates a <ul>
which requires <li>
children, and if the children are groups/dividers, those are not <li>
elements. I think we'll want to do something like
- omit this here (or move it)
- make sure there isn't any styling or JS that needs
.ws-side-nav-list
on this particular<NavList>
for something crucial - if there is make sure we keep.ws-side-nav-list
where it's needed - wrap the nav items created in this block in a
<NavList>
patternfly-org/packages/documentation-framework/components/sideNav/sideNav.js
Lines 236 to 239 in 1beb389
) : NavItem({ key: processed.href, text: processed.text || capitalize(processed.href.replace(/\//g, '').replace(/-/g, ' ')), href: processed.href
Feels like cursor did some extra things -- I asked it to review its own work and make sure any efficiency/accessibility problems are addressed, but lmk if it went to far. Tested with the nav array and it seems to still work fine |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would maybe re-approach this in a way that doesn't require as much logic being added in, i.e. having the grouped nav sections get defined as something like
sideNavItems: [
{
title: 'Learn',
items: [{ section: 'About us' }, { section: 'Releases' }, { section: 'get-started' }],
hasDivider: true
},
{
title: 'Design and develop',
items: [
{ section: 'components' },
{ section: 'patterns' },
{ section: 'extensions' },
{ section: 'foundations' },
{ section: 'accessibility' },
{ section: 'AI' },
{ section: 'content' },
{ section: 'developer-guides' }
],
hasDivider: true
},
{ title: 'Connect', items: [{ section: 'get-involved' }, { section: 'get-help' }] }
],
That would let us just handle it on the sideNav.js
side by sorting them with something like
const groupedItems = navItems.filter((entry) => entry.title);
const ungroupedItems = navItems.filter((entry) => !entry.title);
WDYT?
@wise-king-sullyman I think that format makes a lot more sense! The flat structure felt a little funky to me, but I wasn't sure. Lmk what you think about my last update. I added support for the structure you mentioned and also kept support for the flat structure since I'm not adding the groups until later in the implementation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Awesome work on this 🥳
Closes #4750
This PR is only meant to add the functionality, actually adding in nav groups (I've tested with success locally) will come in a follow up pr for #4753
this is how I implemented in docs-site to test: