NavList API by colebemis · Pull Request #2027 · primer/react

colebemis · 2022-04-18T23:22:38Z

In the spirit of documentation-driven development (DDD), I've drafted the documentation for the NavList (previously NavigationList) component. I'm looking for feedback on the component API before I begin implementation.

✨ Direct link to the docs preview

Code review notes

We renamed NavigationList to NavList to save characters and align with UnderlineNav and the <nav> element (Slack discussion). The NavigationList ViewComponent will be renamed to NavList when it's moved from dotcom into the primer/view_components repo.
I'd love to have an accessibility expert verify that the "Rendered HTML" sections look correct.
I'm not confident in the sub-item API yet. Extra consideration for that section would be appreciated.
Should title be a required prop on the NavList.Group component?

Part of https://github.com/github/primer/issues/637

changeset-bot · 2022-04-18T23:22:39Z

⚠️ No Changeset found

Latest commit: dc26139

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

github-actions · 2022-04-18T23:26:27Z

size-limit report 📦

Path	Size
dist/browser.esm.js	65.64 KB (0%)
dist/browser.umd.js	65.98 KB (0%)

…igationlist-api

siddharthkp · 2022-04-20T09:38:12Z

+<NavList>
+ <NavList.Item href="/branches">Branches</NavList.Item>
+ <NavList.Item href="/tags">Tags</NavList.Item>
+ <NavList.Item>


(weakly held opinions) Let's talk subnav,
1.1 Making sub item work: Should we give this expandable item a special name?
like NavList.CollapsableItem, NavList.ExpandableItem, NavList.Section, NavList.Accordion, NavList.Details
<NavList><NavList.Itemhref="/branches">Branches</NavList.Item><NavList.Itemhref="/tags">Tags</NavList.Item><NavList.ExpandableItem> Actions <NavList.LeadingVisual></ActionIcon/></NavList.LeadingVisual><NavList.SubItemhref="/actions">General</NavList.SubItem><NavList.SubItemhref="/actions/runners">Runners</NavList.SubItem> </NavList.ExpandableItem /> </NavList>
➕ explicit over implicit leads to more predictable API / readable application code?
➖ subItems and item text/visual as siblings feels off
➖ verbose name
1.2 Making sub item work: Should we give this expandable item a special prop instead?
like <NavList.Item expandable>
<NavList><NavList.Itemhref="/branches">Branches</NavList.Item><NavList.Itemhref="/tags">Tags</NavList.Item><NavList.Itemexpandable> Actions <NavList.LeadingVisual></ActionIcon/></NavList.LeadingVisual><NavList.SubItemhref="/actions">General</NavList.SubItem><NavList.SubItemhref="/actions/runners">Runners</NavList.SubItem> </NavList.Item/> </NavList>
➕ explicit over implicit leads to more predictable API / readable application code?
➕ minimal API
➖ the extra prop doesn't really do anything other than make the intent clear
➖ subItems and item text/visual as siblings feels off
2. Forget SubItem, wrap items in NavList.SubNav
<NavList><NavList.Itemhref="/branches">Branches</NavList.Item><NavList.Itemhref="/tags">Tags</NavList.Item><NavList.Item> Actions <NavList.LeadingVisual></ActionIcon/></NavList.LeadingVisual><NavList.SubNav><NavList.Itemhref="/actions">General</NavList.Item><NavList.Itemhref="/actions/runners">Runners</NavList.Item></NavList.SubNav> </NavList.Item/> </NavList>
➕ items wrapped in SubNav feels slightly better than direct siblings to item text/visual
➕ the same NavList.Item which accepts the same props*
➕ explicit over implicit leads to more predictable API / readable application code?
➖ verbose API
➖ does not match view-components API
Proir art: auth0/cosmos; API | Visual
/* assuming that's good, and subitem does not need to restricted to just text

Looking at other primer implementations for inspiration,
primer/css uses .ActionList-item .ActionList-item--hasSubItem
primer/view-components seems to infers it from children
As the view-components implementation is marked as experimental, I think it's okay to diverge from it at this point if we can find an API that works for both

As the view-components implementation is marked as experimental, I think it's okay to diverge from it at this point if we can find an API that works for both
Agree!
Making sub item work: Should we give this expandable item a special name?
Initially I kept Item as generic in PCSS, but eventually ended up creating ItemCollapsible as a separate entity. It felt easier to work with as its own component, but I don't have a strong preference over any of the options @siddharthkp listed above 😄

This is amazing feedback. Thank you @siddharthkp! I've scheduled some time to discuss the nuances of these API options. Feel free to move the calendar event if another time is more convenient :)
Another API consideration: TreeView/TreeGrid will need to support similar expand/collapse functionality. I would love to pick an API that we can use across all three components

/* assuming that's good, and subitem does not need to restricted to just text
Good question! @vdepizzol @langermank Should subitem support all the same functionality as item or is it more restrictive?

@siddharthkp and I tentatively decided to move forward with option 2:
<NavList><NavList.Itemhref="/branches">Branches</NavList.Item><NavList.Itemhref="/tags">Tags</NavList.Item><NavList.Item> Actions <NavList.LeadingVisual></ActionIcon/></NavList.LeadingVisual><NavList.SubNav><NavList.Itemhref="/actions">General</NavList.Item><NavList.Itemhref="/actions/runners">Runners</NavList.Item></NavList.SubNav> </NavList.Item/> </NavList>
If anyone has strong opinions against this API, please let me know

As @siddharthkp mentioned, the PVC NavigationList infers whether or not it should expand/collapse by determining if it has any sub-items, i.e. any item that has sub-items can be expanded/collapsed. That seemed like a reasonable assumption at the time. Will there ever be a need for a list of non-collapsible sub-items?
The API you've chosen makes sense, but I do wonder if simply using <NavList.SubItem ...> would be closer to the PVC API. For example:
<NavList><NavList.Itemhref="/branches"> Branches <NavList.SubItemhref="/actions">General</NavList.SubItem><NavList.SubItemhref="/actions/runners">Runners</NavList.SubItem> </NavList.Item/> </NavList>
My understanding is navigation lists only ever have one nesting level.

My understanding is navigation lists only ever have one nesting level.
I think you're correct. We're working on a TreeView component to support more levels of nesting.

I do wonder if simply using <NavList.SubItem ...> would be closer to the PVC API
@siddharthkp and I are leaning towards the <NavList.SubNav> component because it will allow us to create a predictable and consistent API across NavList, TreeView, and TreeGrid. Would a similar API work for ViewComponents?

@colebemis yeah that works for me 😄 Just as long as it's not expected that SubNavs are a whole nav unto themselves, i.e. can themselves have sub navs, etc.

langermank

Looking good!

langermank · 2022-04-20T15:58:28Z

+<NavList>
+ <NavList.Item href="/branches">Branches</NavList.Item>
+ <NavList.Item href="/tags">Tags</NavList.Item>
+ <NavList.Item>


As the view-components implementation is marked as experimental, I think it's okay to diverge from it at this point if we can find an API that works for both
Agree!
Making sub item work: Should we give this expandable item a special name?
Initially I kept Item as generic in PCSS, but eventually ended up creating ItemCollapsible as a separate entity. It felt easier to work with as its own component, but I don't have a strong preference over any of the options @siddharthkp listed above 😄

langermank · 2022-04-20T16:00:02Z

+ <li><a href="/">Dashboard</a></li>
+ <li><a href="/pulls">Pull requests</a></li>
+ <li><a href="/issues">Issues</a></li>
+ <div role="separator"></div>


I think you need aria-hidden="true" here as well

I'm curious what purpose role would serve if aria-hidden is set to true cc @ichelsea

Good question, I think I added that based on the a11y audit but didn't research it myself.

cc: @alliethu I'm not entirely sure the nuances on this one

camertron · 2022-04-21T22:53:28Z

+
+```jsx
+<NavList>
+ <NavList.Group title="Overview">


I assume groups in NavList are analogous to the concept of sections in NavigationList? If so, why not call them sections in NavList as well?

I called it Group because that what it's called inActionList: https://primer.style/react/ActionList#with-groups
It looks like "section" and "group" are used interchangeably in some of our docs. I propose that we standardize on "group" in Primer React and ViewComponents because it aligns with the ARIA role (group). Here's some more context

camertron

Looks good to me, just left a few non-blocking comments. One remaining question: why is the component called NavList instead of NavigationList?

siddharthkp

mperrotti · 2022-04-25T17:42:41Z

+<details>
+ <summary>Rendered HTML</summary>


Is the <details> and <summary> content meant to be inside the html codefence?

No. These were a temporary way to get review on the rendered HTML without cluttering the document too much. I'll remove these <details> sections when I implement the component.

mperrotti · 2022-04-25T17:43:55Z

+
+```html
+<nav>
+ <ul role="list">


Do we need to render role="list" if we're using a <ul>?

There's a bug in some browsers that removes the list semantics from ul if the element has list-style: none (article). role="list" restores the list semantics so it's properly announced by screenreaders. @alliethu or @ichelsea please correct me if I'm wrong 🙏

Yes @colebemis! That's correct 🙂 . It's a failsafe for the browsers that remove it.

colebemis · 2022-04-26T00:38:04Z

why is the component called NavList instead of NavigationList?

@camertron we had a conversation about "NavigationList" vs "NavList" in Slack. The consensus was to rename NavigationList to NavList in both React and ViewComponents to save characters and align with UnderlineNav and the <nav> element

Create NavigationList.mdx
3e92016

colebemis had a problem deploying to needs-approval April 18, 2022 23:22 Failure

colebemis added the skip changeset This change does not need a changelog label Apr 19, 2022

Update NavList docs
6d72e3b

colebemis had a problem deploying to needs-approval April 19, 2022 23:36 Failure

colebemis changed the title ~~[WIP] NavigationList API~~NavList APIApr 19, 2022

colebemis marked this pull request as ready for review April 19, 2022 23:37

colebemis requested review from a team and rezrah April 19, 2022 23:37

Merge branch 'main' into navigationlist-api
c994655

colebemis had a problem deploying to needs-approval April 19, 2022 23:37 Failure

colebemis requested a review from siddharthkp April 19, 2022 23:37

colebemis added 2 commits April 19, 2022 16:40

Fix lint error
9aaafa5

Merge branch 'navigationlist-api' of github.com:primer/react into nav…
6392bb6
…igationlist-api

colebemis had a problem deploying to needs-approval April 19, 2022 23:40 Failure

colebemis added the react label Apr 19, 2022

siddharthkp reviewed Apr 20, 2022
View reviewed changes

langermank reviewed Apr 20, 2022
View reviewed changes

colebemis added 2 commits April 21, 2022 10:22

current prop -> aria-current prop
5d44ec1

Update sub-item API
0489903

colebemis had a problem deploying to needs-approval April 21, 2022 17:23 Failure

colebemis temporarily deployed to github-pages April 21, 2022 17:43 Inactive

camertron reviewed Apr 21, 2022
View reviewed changes

camertron approved these changes Apr 21, 2022
View reviewed changes

siddharthkp approved these changes Apr 22, 2022
View reviewed changes

lesliecdubs assigned siddharthkp and colebemis Apr 25, 2022

mperrotti reviewed Apr 25, 2022
View reviewed changes

mperrotti approved these changes Apr 25, 2022
View reviewed changes

Merge branch 'main' into navigationlist-api
dc26139

colebemis had a problem deploying to needs-approval April 26, 2022 00:39 Failure

colebemis temporarily deployed to github-pages April 26, 2022 00:55 Inactive

colebemis merged commit 1f6f8f7 into mainApr 28, 2022

colebemis deleted the navigationlist-api branch April 28, 2022 17:10

colebemis mentioned this pull request May 5, 2022
Implement NavList component (part 1) #2058
Merged
10 tasks

Uh oh!

Conversation

colebemis commented Apr 18, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code review notes

Uh oh!

changeset-botBot commented Apr 18, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⚠️ No Changeset found

Uh oh!

github-actionsBot commented Apr 18, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

size-limit report 📦

Uh oh!

siddharthkpApr 20, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

1.1 Making sub item work: Should we give this expandable item a special name?

1.2 Making sub item work: Should we give this expandable item a special prop instead?

2. Forget SubItem, wrap items in NavList.SubNav

Uh oh!

siddharthkpApr 20, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

colebemisApr 20, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

camertronApr 26, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

langermank left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

camertron left a comment

Choose a reason for hiding this comment

Uh oh!

siddharthkp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

colebemis commented Apr 26, 2022

colebemis commented Apr 18, 2022•
edited
Loading

changeset-botBot commented Apr 18, 2022•
edited
Loading

github-actionsBot commented Apr 18, 2022•
edited
Loading

siddharthkpApr 20, 2022•
edited
Loading

2. Forget `SubItem`, wrap items in `NavList.SubNav`

siddharthkpApr 20, 2022•
edited
Loading

colebemisApr 20, 2022•
edited
Loading

camertronApr 26, 2022•
edited
Loading