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

isom-1709 - chore: do not wrap link text in span #965

Conversation

adriangohjw
Copy link
Contributor

Problem

Closes https://linear.app/ogp/issue/ISOM-1709/[lighthouse-seo]-non-dropdown-link-in-navbar-does-not-have-descriptive

Solution

Breaking Changes

  • Yes - this PR contains breaking changes
    • Details ...
  • No - this PR is backwards compatible

Improvements:

  • do not wrap non-dropdown navbar item in <span>

@adriangohjw adriangohjw added the enhancement New feature or request label Dec 29, 2024
@adriangohjw adriangohjw self-assigned this Dec 29, 2024
@adriangohjw adriangohjw requested a review from a team as a code owner December 29, 2024 05:23
Copy link

linear bot commented Dec 29, 2024

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this isn't from this PR but thought to commit it in together

@datadog-opengovsg
Copy link

datadog-opengovsg bot commented Dec 29, 2024

Datadog Report

Branch report: adriangohjw/isom-1709-lighthouse-seo-non-dropdown-link-in-navbar-does-not-have
Commit report: 1c37989
Test service: isomer-studio

✅ 0 Failed, 245 Passed, 36 Skipped, 49.37s Total Time
➡️ Test Sessions change in coverage: 1 no change

@adriangohjw adriangohjw changed the title chore - do not wrap link text in span isom-1709 - chore: do not wrap link text in span Dec 29, 2024
@@ -64,9 +64,9 @@ export const NavItem = forwardRef<HTMLButtonElement, NavbarItemProps>(
isExternal={isExternalUrl(url)}
showExternalIcon={isExternalUrl(url)}
href={referenceLinkHref}
className={item({ isOpen })}
className={`${item({ isOpen })} ${groupFocusVisibleHighlight()}`}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: I think the group focus visible highlight might need to be replaced with the non-group version (focusVisibleHighlight), as it doesn't work currently (but please verify also):

Screen.Recording.2025-01-03.at.11.07.04.mov

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dcshzj address in latest commits, thanks!

@adriangohjw adriangohjw merged commit 9d5256c into main Jan 3, 2025
18 checks passed
@adriangohjw adriangohjw deleted the adriangohjw/isom-1709-lighthouse-seo-non-dropdown-link-in-navbar-does-not-have branch January 3, 2025 05:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants