carbon-design-system / ibm-products

A Carbon-powered React component library for IBM Products
https://carbon-for-ibm-products.netlify.app/
Apache License 2.0
87 stars 120 forks source link

feat(Nav): new component Nav #4420

Closed lily-peng closed 3 months ago

lily-peng commented 3 months ago

Contributes to #4136.

Migrating Nav from Products v1 / Security / Components / Nav.

v1 references: GitHub, Storybook.

What did you change?

This is a new v2 component.

How did you test and verify your work?

github-actions[bot] commented 3 months ago

DCO Assistant Lite bot All contributors have signed the DCO.

netlify[bot] commented 3 months ago

Deploy Preview for carbon-for-ibm-products ready!

Name Link
Latest commit 6dae6447895e94b4365cf244b22565ec15ba3659
Latest deploy log https://app.netlify.com/sites/carbon-for-ibm-products/deploys/65e749d2e955d6000942657a
Deploy Preview https://deploy-preview-4420--carbon-for-ibm-products.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

lily-peng commented 3 months ago

I have read the DCO document and I hereby sign the DCO.

lily-peng commented 3 months ago

recheck

bndege2 commented 3 months ago

Design review:

Noted inconsistencies in focus and Nav interaction Nav spacing active states & spacing.

bndege2 commented 3 months ago

https://github.com/carbon-design-system/ibm-products/assets/157747366/8581c0d5-0812-47ba-a237-e4af1bf86d0e

Nav item 1-2 is triggering Nav list 2 expanded on page load to open when clicked

lily-peng commented 3 months ago
image
matthewgallo commented 3 months ago

Another option could be a render prop, if you need to add props to one of the children. This is used across Carbon (see Dropdown) and is also one of the recommended alternatives to cloneElement and Children.map from the React docs.

lily-peng commented 3 months ago

@davidmenendez The difficulty with line 122 of Nav.js is that the props being added probably aren't helpful if exposed to the consumer, since they are necessary for the component to work as intended, like activeHref to maintain the url so the page doesn't reload on every click. And props with handler functions like onListClick and onItemClick in lines 68 and 69.

If we don't worry about passing in an element in NavItem.js, would we assume all NavItems are a, and therefore kind of eliminates the need for a separate component like NavItemLink?

@matthewgallo I could use render props, but depending on if the child is a list or a list item, they should receive different props, which necessitates some sort of iteration.

Let me know if this makes sense, or if I'm understanding this correctly. Thanks!

lily-peng commented 3 months ago

The plan is to implement the changes and enhancements suggested by David at a later date.

lily-peng commented 3 months ago

Thanks David! It says I'm not authorized to merge the PR though :(

matthewgallo commented 3 months ago

Happy to pop it into the merge queue! 😄