Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Active NavList.Item is incorrectly missing bold styling #4427

Closed
rwe opened this issue Mar 22, 2024 · 4 comments
Closed

Active NavList.Item is incorrectly missing bold styling #4427

rwe opened this issue Mar 22, 2024 · 4 comments
Labels
bug Something isn't working react

Comments

@rwe
Copy link

rwe commented Mar 22, 2024

Description

Active NavList.Items do not have the expected bold styling. Note that this appears to be a bug, diagnosed below, and not browser-specific.

❌ Actual: @primer/react
image

Compare with expected styling on GitHub's own use of the component:

✅ Expected (via github.com):
image

And the screenshot from the NavList docs themselves:

✅ Expected (via docs)
image

Diagnosis

The current/active state of a NavList.Item is controlled by the aria-current property which drives the ActionList.LinkItemActionList.Item's active property. That property then decides whether ActionList.Item's isActive styles are applied:

const activeStyles = {
fontWeight: 'bold',
bg: 'actionListItem.default.selectedBg',
'&::after': {
position: 'absolute',
top: 'calc(50% - 12px)',
left: -2,
width: '4px',
height: '24px',
content: '""',
bg: 'accent.fg',
borderRadius: 2,
},
}

Those styles are all correctly applied to the container LiBox.

However, (unless a "Description" heading element occurs, which is not desired here and is not shown in the above expected examples), fontWeight is overridden immediately:

fontWeight: slots.inlineDescription || slots.blockDescription ? 'bold' : 'normal',

Without being too familiar with the codebase or StyledComponents in general, it seems a possible fix might be something like:

- fontWeight: slots.inlineDescription || slots.blockDescription ? 'bold' : 'normal',
+ fontWeight: slots.inlineDescription || slots.blockDescription ? 'bold' : null,

Steps to reproduce

This is visible in the current storybook: https://primer.style/react/storybook/?path=/story/components-navlist--simple
whose code is given as:

export const Simple: Story = () => (
  <PageLayout>
    <PageLayout.Pane position="start">
      <NavList>
        <NavList.Item href="#" aria-current="page">
          Item 1
        </NavList.Item>
        <NavList.Item href="#">Item 2</NavList.Item>
        <NavList.Item href="#">Item 3</NavList.Item>
      </NavList>
    </PageLayout.Pane>
    <PageLayout.Content></PageLayout.Content>
  </PageLayout>
)

It occurs with any simple NavList with a "current" item, and I believe also with a basic ActionList.Item.

Version

36.9.0

Through 369c303 (at time of issue creation).

Browser

(Not browser-specific)
Chrome

@rwe rwe added the bug Something isn't working label Mar 22, 2024
Copy link
Contributor

Uh oh! @rwe, the image you shared is missing helpful alt text. Check your issue body.

Alt text is an invisible description that helps screen readers describe images to blind or low-vision users. If you are using markdown to display images, add your alt text inside the brackets of the markdown image.

Learn more about alt text at Basic writing and formatting syntax: images on GitHub Docs.

🤖 Beep boop! This comment was added automatically by github/accessibility-alt-text-bot.

@langermank
Copy link
Contributor

Thanks for reporting this! I'm going to close this as a duplicate of #4367, but there's a fix ready to merge that will probably be in the next release.

@langermank langermank closed this as not planned Won't fix, can't repro, duplicate, stale Mar 22, 2024
Copy link
Contributor

Uh oh! @rwe, the image you shared is missing helpful alt text. Check your issue body.

Alt text is an invisible description that helps screen readers describe images to blind or low-vision users. If you are using markdown to display images, add your alt text inside the brackets of the markdown image.

Learn more about alt text at Basic writing and formatting syntax: images on GitHub Docs.

🤖 Beep boop! This comment was added automatically by github/accessibility-alt-text-bot.

@rwe
Copy link
Author

rwe commented Mar 22, 2024

@langermank Oh wow, I browsed through the issues but apparently just didn't read closely enough! That does indeed appear to be a dupe.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working react
Projects
None yet
Development

No branches or pull requests

2 participants