[WIP] Replace UnderlineNav component by pksjce · Pull Request #2017 · primer/react

pksjce · 2022-04-11T09:30:12Z

Describe your changes here.

This PR implements new version of UnderlineNav as per https://github.com/github/primer/issues/892

Why is this work being done?
The current UnderlineNav is outdated and not adequate in the following ways.

Uses css class names. These clash with existing implementations.
Not accessible as it doesnt set aria-label and aria-current-page.
Does not support icons
Has no support for smaller screens

My proposal with this PR is

We can keep the existing API for UnderlineNav while changing all of its internals and adding new API for icons and accessibility
No deprecation and drafts required.
Refer to https://primer.style/view-components/components/alpha/underlinenav/#

Pending things to merge this PR.

Closeshttps://github.com/github/primer/issues/892

Screenshots

Please provide before/after screenshots for any visual changes

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-04-11T09:30:14Z

⚠️ No Changeset found

Latest commit: 191e43d

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-11T09:33:38Z

size-limit report 📦

Path	Size
dist/browser.esm.js	65.75 KB (0%)
dist/browser.umd.js	66.15 KB (0%)

langermank · 2022-04-11T21:13:22Z

Hi @pksjce! We have an upcoming major release for PCSS that includes some visual improvements for UnderlineNav. Its nothing drastic, but does improve the experience when items are focused. It also upstreams some CSS from dotcom that uses CSS content to create a "fake" bold state, which prevents layout shift when an item becomes active (which I noticed it happening in PRC currently.) primer/css#1821

Let me know if you want to sync on any of this, happy to chat! I'm not sure the scope of this work either but wanted to make sure this release from PCSS is on your radar.

colebemis · 2022-04-12T21:12:11Z

+ paddingX: get('space.2'),
+ paddingY: get('space.3'),
+ marginRight: get('space.3'),
+ fontSize: get('fontSizes.1'),


get() shouldn't be necessary since this object is passed to the sx prop

I'm not sure. whats the alternative? It doesn't seem to work without the get.

colebemis · 2022-04-12T21:12:23Z

+ }
+ const iconWrapStyles = {
+ display: 'inline-block',
+ marginRight: '8px'


Suggested change
marginRight: '8px'
marginRight: 2

colebemis · 2022-04-12T21:13:49Z

+export const UnderlineNavLink = ({
+ sx: sxProp = {},
+ href = '#',
+ children,
+ selected = false,
+ leadingIcon: LeadingIcon,
+ ...props


Should we support the as prop to allow people to use React Router-like link components?

pksjce · 2022-04-14T09:15:18Z

Here's a loom of me demo-ing what's possible with the new implementation of UnderlineNavhttps://www.loom.com/share/0a5432ffced946c5901109882e045b8d

pksjce · 2022-05-09T13:18:44Z

Working on making a responsive UnderlineNav

Add overflow prop as per discussion with design
Add context to store child widths
Create useNavResponsive (maybe change this name) for managing container width.

maximedegreve · 2022-05-13T13:34:26Z

Hi @pksjce

Thank you for the video 🎊 I've noticed that the responsive behaviour behaves a lot different to what we do right now in repositories

In repositories:

We only show the 3 dots more button when the content overflows. If the content fits we never show the 3 dots more button.

You might want to sync up with @langermank or @vdepizzol on this behaviour as I saw designs where we add an additional more item to the end of the nav items when the content doesn't fit instead of the 3 dots button.

Example:

maximedegreve · 2022-05-25T18:28:04Z

Hi @pksjce is there any update on above?

pksjce · 2022-06-23T14:09:27Z

Closing this as I'm continuing the work in #2142

pksjce requested review from a team and colebemis April 11, 2022 09:30

pksjce had a problem deploying to needs-approval April 11, 2022 09:30 Failure

pksjce had a problem deploying to needs-approval April 11, 2022 09:32 Failure

pksjce changed the title ~~Replace UnderlineNav component~~[WIP] Replace UnderlineNav componentApr 11, 2022

pksjce requested a review from maximedegreve April 11, 2022 09:32

pksjce self-assigned this Apr 11, 2022

pksjce added the react label Apr 11, 2022

pksjce added this to the FY22 - Q4 milestone Apr 11, 2022

pksjce marked this pull request as draft April 11, 2022 09:33

pksjce force-pushed the pk/underline-nav branch from 375da55 to 0f27ee8Compare April 11, 2022 09:33

pksjce temporarily deployed to needs-approval April 11, 2022 09:33 Inactive

colebemis reviewed Apr 12, 2022
View reviewed changes

pksjce force-pushed the pk/underline-nav branch from 0f27ee8 to a7c77f6Compare April 13, 2022 10:01

pksjce had a problem deploying to needs-approval April 13, 2022 10:01 Failure

pksjce had a problem deploying to needs-approval April 14, 2022 08:58 Failure

pksjce force-pushed the pk/underline-nav branch from bb741c0 to 47c9c62Compare May 9, 2022 13:16

pksjce had a problem deploying to needs-approval May 9, 2022 13:16 Failure

pksjce temporarily deployed to github-pages May 9, 2022 13:35 Inactive

pksjce added 5 commits May 16, 2022 18:17

Replace UnderlineNav component
b94bcff

Remove existing component
12a8043

Add types
bb06609

Add aaria label and aria-current properties
b29b7d0

Add UnderlineNavActions to support responsive behaviour
6e47f8f

pksjce added 2 commits May 16, 2022 18:17

Add documentation and actions
9a90b2b

Make underlineNav responsive - part 1
eab2565

pksjce added 4 commits June 15, 2022 20:58

Split to items and actions
e90f7c2

Calculate the right width for the overflow items
e783fd2

Convert UnderlineNav Link to ActionList Item
ab40c5c

Horizontal scroll and resizeobserver support
191e43d

pksjce force-pushed the pk/underline-nav branch from 47c9c62 to 191e43dCompare June 21, 2022 14:05

pksjce had a problem deploying to needs-approval June 21, 2022 14:05 Failure

pksjce temporarily deployed to github-pages June 21, 2022 14:11 Inactive

pksjce mentioned this pull request Jun 23, 2022
Draft component for Underline Nav #2142
Merged
6 tasks

pksjce closed this Jun 23, 2022

joshblack deleted the pk/underline-nav branch January 19, 2023 16:48

Navigation Menu

Uh oh!

[WIP] Replace UnderlineNav component - #2017

[WIP] Replace UnderlineNav component#2017
pksjce wants to merge 11 commits into
mainfrom
pk/underline-nav

pksjce commented Apr 11, 2022•
edited
Loading

Uh oh!

changeset-botBot commented Apr 11, 2022•
edited
Loading

Uh oh!

github-actionsBot commented Apr 11, 2022

Uh oh!

langermank commented Apr 11, 2022

Uh oh!

colebemisApr 12, 2022

Uh oh!

pksjceApr 14, 2022

Uh oh!

colebemisApr 12, 2022

Uh oh!

colebemisApr 12, 2022

Uh oh!

pksjce commented Apr 14, 2022

Uh oh!

pksjce commented May 9, 2022

Uh oh!

maximedegreve commented May 13, 2022•
edited
Loading

Uh oh!

maximedegreve commented May 25, 2022

Uh oh!

pksjce commented Jun 23, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

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

Uh oh!

Screenshots

Merge checklist

Uh oh!

changeset-botBot commented Apr 11, 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 11, 2022

size-limit report 📦

Uh oh!

langermank commented Apr 11, 2022

Uh oh!

colebemisApr 12, 2022

Choose a reason for hiding this comment

Uh oh!

pksjceApr 14, 2022

Choose a reason for hiding this comment

Uh oh!

colebemisApr 12, 2022

Choose a reason for hiding this comment

Uh oh!

colebemisApr 12, 2022

Choose a reason for hiding this comment

Uh oh!

pksjce commented Apr 14, 2022

Uh oh!

pksjce commented May 9, 2022

Working on making a responsive UnderlineNav

Uh oh!

maximedegreve commented May 13, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maximedegreve commented May 25, 2022

Uh oh!

pksjce commented Jun 23, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

pksjce commented Apr 11, 2022•
edited
Loading

changeset-botBot commented Apr 11, 2022•
edited
Loading

maximedegreve commented May 13, 2022•
edited
Loading