-
Notifications
You must be signed in to change notification settings - Fork 13
feat(high contrast): add border to some hover states #91
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
Conversation
|
The build for this is failing, and cursor says
and recommends adding before running |
thatblindgeye
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be good to rebase after a PR for workflows was merged in. Just a note below.
packages/module/package.json
Outdated
| "@patternfly/react-core": "^6.4.0", | ||
| "@patternfly/react-styles": "^6.4.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nicole had suggested I try keeping the patternfly versions in extensions as what they were, and just updating the docs framework version to the latest (6.28.9). We could try that here so that at least the docs build will allow showing the high contrast styles.
We would also probably want to add to the patternfly-docs.config.js file the following to get the switchers showing up and make it easier to test HC styles:
hasThemeSwitcher: true,
hasHighContrastSwitcher: true
|
|
||
| &:hover { | ||
| background-color: var(--pf-t--global--background--color--action--plain--hover); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lboehling @thatblindgeye I didn't add the hover background, just the outline. Git blame points to a change by @nicolethoen - anyone know why this was added?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is this issue that might handle this: #88
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So it looks like maybe it was applied to the filter side panel by accident and was only meant to be the vertical tabs?
#73 comments talk about a hover background on vertical tabs but it was also applied to the filter side panel, perhaps erroneously?
|
@thatblindgeye IDK if I still need to revert some of the dependency stuff or not? |
mcoker
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one change por favor. Also want to double check that this visual diff looks OK. From what I see, this change:
-
Indents the entire list so that the blue line is not flush-left with the left edge of the container.
-
Reduces the padding around the nav link/box. That makes the layout space a bit more compact. It also changes the link padding so that it matches the height of the blue accent, instead of being taller than the accent.
-
Moves the blue accent outside of the link box
| bottom: 0; | ||
| border-width: var(--pf-t--global--border--width--high-contrast--regular); | ||
| border-style: solid; | ||
| border-color: transparent; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a nit, we can use the action-plain border width token and change it for :hover below
| border-color: transparent; | |
| inset: 0; | |
| border: var(--pf-t--global--border--width--action--plain--default) solid var(--pf-t--global--border--color--high-contrast); |
| text-decoration: none; | ||
|
|
||
| &::after { | ||
| border-color: var(--pf-t--global--border--color--high-contrast); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| border-color: var(--pf-t--global--border--color--high-contrast); | |
| border-width: var(--pf-t--global--border--width--action--plain--hover); |
For clarity - what you see in this PR should hopefully match what you currently see on https://www.patternfly.org/extensions/catalog-view/vertical-tabs There's a whole bunch of custom styling done on .org that's now in the extension. |
mcoker
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
mcoker
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
@thatblindgeye @lboehling just want to double check that this PR will introduce a few changes.
Here are some screenshots Before:
After:
|
thatblindgeye
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the above changes are probably mostly fine, just wondering if the accent being indented along with nested items will be an issue for consumers/other designers who may prefer the accent being consistently placed along the left-most edge of the container.
Maybe we don't change anything for now, but do we think it'd be an easy lift if we have to come back to maybe make that aspect an opt-in?
|
I don't think it would be hard since it was like that minus the .org styling - this is how it appears on .org now since there was local styling applied for whatever reason. |
|
🎉 This PR is included in version 6.3.0-prerelease.3 🎉 The release is available on: Your semantic-release bot 📦🚀 |










Fixes #87
Adds a high contrast border on a pseudoelement for filter side panel and vertical tabs when hovered.
Will need PatternFly 6.4 for the high contrast tokens.
Since there is no theme switcher, add
pf-v6-theme-high-contrastto the html tag to trigger high contrast mode.