feat(dual-list-selector): add high-contrast - #7686
Merged
mcoker merged 8 commits intoAug 25, 2025
Merged
Conversation
Collaborator
|
Preview: https://pf-pr-7686.surge.sh A11y report: https://pf-pr-7686-a11y.surge.sh |
lboehling
approved these changes
Aug 5, 2025
mcoker
requested changes
Aug 12, 2025
Comment on lines
+65
to
+66
| --#{$dual-list-selector}__list-item-row--m-selected--BorderWidth: var(--pf-t--global--border--width--strong); | ||
| --#{$dual-list-selector}__list-item-row--m-selected--BorderColor: var(--pf-t--global--border--color--high-contrast); |
Contributor
There was a problem hiding this comment.
Not sure you need this if it's already defined?
Suggested change
| --#{$dual-list-selector}__list-item-row--m-selected--BorderWidth: var(--pf-t--global--border--width--strong); | |
| --#{$dual-list-selector}__list-item-row--m-selected--BorderColor: var(--pf-t--global--border--color--high-contrast); | |
| --#{$dual-list-selector}__list-item-row--m-selected--BorderWidth: var(--pf-t--global--border--width--strong); |
Comment on lines
+60
to
+61
| --#{$dual-list-selector}__list-item-row--BorderWidth: var(--pf-t--global--border--width--regular); | ||
| --#{$dual-list-selector}__list-item-row--BorderColor: transparent; |
Contributor
There was a problem hiding this comment.
Can we update this so the border-width is what enables the border?
Suggested change
| --#{$dual-list-selector}__list-item-row--BorderWidth: var(--pf-t--global--border--width--regular); | |
| --#{$dual-list-selector}__list-item-row--BorderColor: transparent; | |
| --#{$dual-list-selector}__list-item-row--BorderWidth: 0; | |
| --#{$dual-list-selector}__list-item-row--BorderColor: var(--pf-t--global--border--color--high-contrast); |
| --#{$dual-list-selector}__list-item-row--BorderWidth: var(--pf-t--global--border--width--regular); | ||
| --#{$dual-list-selector}__list-item-row--BorderColor: transparent; | ||
| --#{$dual-list-selector}__list-item-row--hover--BackgroundColor: var(--pf-t--global--background--color--primary--hover); | ||
| --#{$dual-list-selector}__list-item-row--hover--BorderColor: var(--pf-t--global--border--color--high-contrast); |
Contributor
There was a problem hiding this comment.
Suggested change
| --#{$dual-list-selector}__list-item-row--hover--BorderColor: var(--pf-t--global--border--color--high-contrast); | |
| --#{$dual-list-selector}__list-item-row--hover--BorderWidth: var(--pf-t--global--border--width--regular); |
| &:focus { | ||
| &:focus-visible { | ||
| --#{$dual-list-selector}__list-item-row--BackgroundColor: var(--#{$dual-list-selector}__list-item-row--hover--BackgroundColor); | ||
| --#{$dual-list-selector}__list-item-row--BorderColor: var(--#{$dual-list-selector}__list-item-row--BorderColor); |
Member
Author
There was a problem hiding this comment.
Just keeping you on your toes.
srambach
force-pushed
the
7617-dual-list-hi-c
branch
from
August 15, 2025 14:47
6a24bfb to
01757e1
Compare
mcoker
reviewed
Aug 21, 2025
mcoker
left a comment
mcoker
left a comment
Contributor
There was a problem hiding this comment.
Just a few places we can use the action--plain border tokens.
mcoker
force-pushed
the
7617-dual-list-hi-c
branch
from
August 21, 2025 23:15
1299fca to
691ed46
Compare
Contributor
|
Looks like this branch may have been branched from the first commit in https://github.com/srambach/patternfly/tree/7616-drawer-hi-c for #7683. Rebased and dropped the 1st and 4th commits below since they were drawer changes. After the rebase, it exposed that we were including empty "count" badges in all items, so put those in a conditional. Commits before After |
mcoker
approved these changes
Aug 21, 2025
Contributor
|
@srambach do you mind giving this a once over and make sure it looks like you expect? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #7617
Figma design: https://www.figma.com/design/wKcOq7IrzfL1gEK2mocd6r/Semantic-dimension-border-width----font-weight-adjustment-test?node-id=10994-57326&t=r888cXH4fIPuJcgE-4
Top/bottom 1px border on hover (along with background change)
Top/bottom 2px border for selected items UNLESS there's a checkbox
Moves outline on the container out 2px so that the focus outline is not covered by the item's background. Question - use a spacer here or hard-coded pixels? It's not really a logical number.
Changes :focus to :focus-visible because the current implementation leaves a background hanging around if you click something to UNselect it and then move the mouse away.
Note: the border width of list items changes when selected, so we still need to use the pseudoelement
Assisted by Cursor autocomplete