UnderlineNav with updated design for states and counter #2277

broccolinisoup · 2022-08-26T09:10:55Z

UnderlineNav has updated design and this PR is for the updated states (default, selected, hover and focus state) as well as adding counter as a prop.

Screenshots

Screen Recording

UnderlineNav.focus._.hover._.selected.states.mp4

Merge checklist

Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.

changeset-bot · 2022-08-26T09:10:58Z

🦋 Changeset detected

Latest commit: 0699ae0

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@primer/react	Minor

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

github-actions · 2022-08-26T09:15:15Z

size-limit report 📦

Path	Size
dist/browser.esm.js	76.29 KB (0%)
dist/browser.umd.js	76.9 KB (0%)

src/UnderlineNav2/UnderlineNavItem.tsx

pksjce · 2022-08-30T04:23:21Z

src/UnderlineNav2/index.ts


 const UnderlineNav = Object.assign(Nav, {
-  Link: UnderlineNavLink
+  Item: UnderlineNavItem


This would be a breaking change wrt to the old UnderlineNav. We will need to mention it in the changelog.

pksjce

The focus rings work so well! 💯

I can see two things out of place.

The underline from the nav seems to have gone away.
The selected underline seems a bit longer on both sides compared to the Item.

Also I'm sceptical about the name change from UnderlineNav.Link to UnderlineNav.Item. It would break existing API. Though, I agree its a change that makes full sense. Not sure how to proceed with it honestly.

broccolinisoup · 2022-08-30T06:20:54Z

Thanks for the review @pksjce !

The underline from the nav seems to have gone away.

I used the states reference from Figma and there is no underline there so I though this line must have been removed with the updated design but for all the other references in Figma, the line is there! I added it back ✨ Sorry for the confusion!

The selected underline seems a bit longer on both sides compared to the Item.

Yes, you are right! I might have missed that. I also realised there was way too much margin between the li items. So all together, I re-styled them and this is for the reference to compare before and after. (I updated the PR description as well with the new screenshots)

Before

After

And the design

Also I'm sceptical about the name change from UnderlineNav.Link to UnderlineNav.Item. It would break existing API. Though, I agree its a change that makes full sense. Not sure how to proceed with it honestly.

Yeah I agree. It makes sense to me too and I see other "new" components such as ActionList uses Item too so it is good to be consistent. I guess we can discuss with others as well and find the best strategy to move forward with the major release.

danielguillan · 2022-09-01T09:31:00Z

Sorry for the confusion about the removal of the underline.

I initially intentionally removed it so that with the removal of actions, consumers could still easily build a custom UnderlineNav + actions horizontal layout and only deal with the bottom border on the parent container. I also did so to support the use case in the code clone dropdown where we don't use a border at all.

But now that we have a clearer idea of what needs to be supported (and not) and how we'll move forward in the mid-long term, we should bring the underline back.

danielguillan · 2022-09-01T09:40:38Z

I think the name change from UnderlineNav.Link to UnderlineNav.Item makes sense. Please feel free to point anyone interested in this discussion to the design API proposal: https://github.com/github/primer/pull/1239

src/UnderlineNav2/UnderlineNavItem.tsx

broccolinisoup · 2022-09-05T00:59:11Z

src/UnderlineNav2/UnderlineNav.tsx

          <Box as="ul" sx={merge<BetterSystemStyleObject>(overflowStyles, ulStyles)}>
            {responsiveProps.items}
          </Box>

          {actions.length > 0 && (
            <ActionMenu>
-              <ActionMenu.Button>More</ActionMenu.Button>
+              <ActionMenu.Button sx={{m: 0}}>More</ActionMenu.Button>


to fix extra margin around the button on safari

wow, can you add a comment here about this?

pksjce · 2022-09-05T03:06:56Z

src/UnderlineNav2/UnderlineNavItem.tsx

+  /**
+   * Counter
+   */
+  counter?: number


@danielguillan - Wondering if we are going to support loading state for counters.

Just making a comment here for the types. I added the counter prop with only number type just to make the Counter component redundant. We can add another type for the loading state if we are going to support 😊

Yes, we are going to support loading states. We can add support incrementally in another PR if that makes things easier.

pksjce · 2022-09-05T07:16:10Z

.changeset/thirty-mayflies-travel.md

@@ -0,0 +1,5 @@
+---
+'@primer/react': major


I don't think this should be a major change yet. We'll kind of block ourselves doing that. Once we make all the changes, we can graduate from drafts to main folder with a major change. Could you do that before merging?

broccolinisoup temporarily deployed to github-pages August 26, 2022 09:26 Inactive

broccolinisoup changed the title ~~UnderlineNav with updated design for states and counter~~ [WIP]: UnderlineNav with updated design for states and counter Aug 26, 2022

broccolinisoup temporarily deployed to github-pages August 26, 2022 12:11 Inactive

broccolinisoup temporarily deployed to github-pages August 29, 2022 02:12 Inactive

broccolinisoup temporarily deployed to github-pages August 29, 2022 04:58 Inactive

broccolinisoup temporarily deployed to github-pages August 29, 2022 05:10 Inactive