feat(chrome)!: improves markup structure for Nav items

[geotrev]

Description

Introduces Nav.List subcomponent for a semantically accurate navigation list.

Detail

• introduced Nav.List as a wrapper for Nav.Item
• React Context is used to detect if Nav.Item is within a Nav.List, and if so, includes a styled li
• Requires consumers to keep logo/brandmark items out of Nav.List

New composition API:

<Chrome>
  <Nav> // <nav>
    <Nav.Item hasLogo /> // div
    <Nav.List> // <ul>
      <Nav.Item /> // <li>
      <Nav.Item />
      <Nav.Item />
    </Nav.List>
    <Nav.Item hasBrandmark /> // div
  </Nav>
</Chrome>

Checklist

• 👌 design updates will be Garden Designer approved (add the designer as a reviewer)
• 🌐 demo is up-to-date (npm start)
• ⬅️ renders as expected with reversed (RTL) direction
• 🤘 renders as expected with Bedrock CSS (?bedrock)
• 💂‍♂️ includes new unit tests. Maintain existing coverage (always >= 96%)
• ♿ tested for WCAG 2.1 AA accessibility compliance
• 📝 tested in Chrome, Firefox, Safari, and Edge

[coveralls]

coverage: 96.09% (-0.1%) from 96.234%
when pulling 8896a4d on george/chrome-navitems
into 9f3aa37 on next.

[ze-flo]

CSS order rules can lead to accessibility problems and I've had mixed experiences with them. Do we still need these rules or should we leave it to the user to specify the correct order (potential breaking change 💥)?

https://developer.mozilla.org/en-US/docs/Web/CSS/order#accessibility_concerns
https://www.w3.org/TR/css-flexbox-1/#order-accessibility

[geotrev]

Actually, you just jogged my senses a bit. Since this element is just a ul and it's a container, it shouldn't land in an unexpected render order. So, we can probably just remove these I think? 🤔

I'll give it a quick test to confirm, and we can discuss inconsistencies if there are any!

[geotrev]

I think I see why order is used here.

It's to ensure that a child composition of:

<Nav>
  <Nav.List>
    <Nav.Item hasBrandmark />
    <Nav.Item>
      <Nav.ItemText />
      <Nav.ItemIcon />
    </Nav.Item>
    <Nav.Item hasLogo />
  </Nav.List>
</Nav>

Renders in the expected way in the DOM:

<nav>
  <ul>
    <li class="StyledLogoItem /> <!-- logo first -->
    <li class="StyledItem">
      <span class="StyledItemIcon" /> <!-- icon first -->
      <span class="StyledItemText" />
    </li>
    <li class="StyledBrandmarkItem" /> <!-- brandmark last -->
  </ul>
</nav>

E.g., a break-prevention measure. I can't speak to if this is still desirable though. I assume so but maybe JZ or design have further insights. I would agree it is a bit heavy-handed but I can understand the logic after toying with it.

[geotrev]

And now I see that removing order: -1 from <Nav> also changes the above ^ behavior in this PR vs. main/next, which I'll revert.

[ze-flo]

Thanks for your excellent explanation. 💯 It looks like removing order isn't so trivial after all and definitely falls out of scope for this PR. Perhaps something we can revisit in the future.

[geotrev]

Yeah, I agree on revisiting if it makes sense!

[jzempel]

The order CSS throughout Chrome is intentional, and despite the a11y concerns we keep it because enforcing consistent UX tab ordering despite consumer usage is the more important concern. Perhaps if/when a grid layout vs. flex is explored in the future, we can reevaluate.

[ze-flo]

LGTM 🔥 🚀

[jzempel]

In accordance with the a11y issue we're attempting to address, we're looking for logo and brandmark items to live outside the list. Expecting to see:

<Nav>
  <Nav.Item hasLogo />
  <Nav.List>
    <Nav.Item ... />
    <Nav.Item ... />
    <Nav.Item ... />
  </Nav.List>
  <Nav.Item hasBrandmark />
</Nav>

This means <Nav.Item> will need context to understand whether to render a <li> or not.

[geotrev]

Thanks @jzempel - that detail wasn't immediately clear to me from our previous conversations. I recall you mentioned context but I didn't connect that to the logo/brandmark items being excluded from the list. Should be updated now.

[jzempel]

Approved, with minor URL comment for consideration.

[jzempel]

Should this be a relative path so that it is valid whether on next or main?

[geotrev]

Oh, I wasn't sure if that was possible here. Good call. I'll update and test it before I merge!

[ze-flo]

For consistency, it might be good to use the dot notation in this story:

react-components/packages/avatars/demo/~patterns/stories/ChromeStory.tsx

Lines 18 to 28 in 84f57d0

	<HeaderItem aria-label="Products">
	<HeaderItemIcon>
	<Icon />
	</HeaderItemIcon>
	</HeaderItem>
	<HeaderItem isRound aria-label="User profile">
	<Avatar {...args} size="extrasmall">
	<img alt="Example User" src="images/avatars/chrome.png" />
	</Avatar>
	</HeaderItem>
	</Header>

[geotrev]

Dang @ze-flo amazing snipe on that pattern file! haha