8000 fix(nav): remove underline on hover by mcoker · Pull Request #6787 · patternfly/patternfly · GitHub
[go: up one dir, main page]
Skip to content

fix(nav): remove underline on hover#6787

Merged
mcoker merged 2 commits intopatternfly:v6from
mcoker:issue-6765
Jun 14, 2024
Merged

fix(nav): remove underline on hover#6787
mcoker merged 2 commits intopatternfly:v6from
mcoker:issue-6765

Conversation

@mcoker
Copy link
Contributor
@mcoker mcoker commented Jun 13, 2024

fixes #6765

@patternfly-build
Copy link
Collaborator
patternfly-build commented Jun 13, 2024 •
edited
Loading

Preview: https://patternfly-pr-6787.surge.sh

A11y report: https://patternfly-pr-6787-a11y.surge.sh

srambach
srambach approved these changes Jun 13, 2024
View reviewed changes
Copy link
Member
@srambach srambach left a comment

Choose a reason for hiding this comment

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

Delineful!

mattnolting
mattnolting approved these changes Jun 13, 2024
View reviewed changes
Copy link
Collaborator
@mattnolting mattnolting left a comment

Choose a reason for hiding this comment

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

Looks like we apply text-decoration: none; to the interactive element itself within several components (icon, jump-links, menu-item, simple-list-item-link, tab-link) and on :hover/:focus in others (breadcrumb-link, label, toggle-group). I prefer setting on the element itself, but we use both, so not a blocker. WDYT about moving to the element's default state?

andrew-ronaldson
andrew-ronaldson approved these changes Jun 13, 2024
View reviewed changes
Copy link
Collaborator
@andrew-ronaldson andrew-ronaldson left a comment

Choose a reason for hiding this comment

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

You've crossed the line from awesome to super hero!

@mattnolting
Copy link
Collaborator
mattnolting commented Jun 13, 2024

Also, should you use the semantic token --pf-t--global--link--text-decoration instead of text-decoration: none?

@mcoker
Copy link
Contributor Author
mcoker commented Jun 13, 2024

Also, should you use the semantic token --pf-t--global--link--text-decoration instead of text-decoration: none?

@mattnolting I don't think so, reason being --pf-t--global--link--text-decoration is the token/setting for a link's default text decoration. If you were to set that to something custom in an app, we wouldn't want that styling to apply to nav/other (more-button-looking links in components) on hover/focus.

@mcoker
Copy link
Contributor Author
mcoker commented Jun 14, 2024

Looks like we apply text-decoration: none; to the interactive element itself within several components (icon, jump-links, menu-item, simple-list-item-link, tab-link) and on :hover/:focus in others (breadcrumb-link, label, toggle-group). I prefer setting on the element itself, but we use both, so not a blocker. WDYT about moving to the element's default state?

@mattnolting nice! We should probably move the rule to the base element. If someone declared --pf-t--global--link--text-decoration to underline regular text links, we wouldn't want that to persist to nav and components like this that we don't want to present like text links 👍👍

@mcoker mcoker requested a review from srambach June 14, 2024 15:23
srambach
srambach approved these changes Jun 14, 2024
View reviewed changes
8000 Copy link
Member
@srambach srambach left a comment

Choose a reason for hiding this comment

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

LPTM 🚀

@mcoker mcoker merged commit a9b79b2 into patternfly:v6 Jun 14, 2024
@patternfly-build
Copy link
Collaborator
patternfly-build commented Jun 14, 2024

🎉 This PR is included in version 6.0.0-alpha.161 🎉

The release is available on:

Your semantic-release bot 📦🚀

@mcoker mcoker mentioned this pull request Jun 17, 2024
Closed
mattnolting pushed a commit to mattnolting/patternfly that referenced this pull request Jun 21, 2024
@mcoker
fix(nav): remove underline on hover (patternfly#6787)
e5341d2 
* fix(nav): remove underline on hover

* fix(nav): move text decoration rule to nav link
@mcoker mcoker deleted the issue-6765 branch January 7, 2025 16:55
@patternfly-build patternfly-build mentioned this pull request Apr 16, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mattnolting mattnolting mattnolting approved these changes

@srambach srambach srambach approved these changes

@andrew-ronaldson andrew-ronaldson andrew-ronaldson approved these changes

Assignees

No one assigned

Labels

released on @alpha

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@mcoker @patternfly-build @mattnolting @srambach @andrew-ronaldson
0