Add consistent keyboard focus ring across interactive components #2050
Conversation
This file contains 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
|
|
fd71696 to
8461e68
Compare
| @@ -31,7 +31,7 @@ $text-colors: ( | |||
| 'danger': #ee0d0d, | |||
| ); | |||
|
|
|||
| $focus-ring-color: rgb(77, 144, 254); | |||
| $focus-ring-color: rgb(34, 139, 236); | |||
There was a problem hiding this comment.
This is the new focus color, as verified by @henrikvoetmand
There was a problem hiding this comment.
Any particular reason this is declared with rgb()? The other color values are declared using hexadecimal. (It was probably not you who introduced this. I'm just curious 🙂)
There was a problem hiding this comment.
Dunno, but changed it to hexadecimal.
4745bfc to
74651f2
Compare
| @@ -32,6 +33,10 @@ | |||
| } | |||
| } | |||
| } | |||
| &.compact { | |||
| //TODO: verify if this is OK. It enables focus ring to not be cut off for first chip | |||
There was a problem hiding this comment.
Handle this TODO. Probably a quick talk with @MadsBuchmann.
There was a problem hiding this comment.
What's the status on this TODO?
There was a problem hiding this comment.
Removed, we'll move forward with this implementation!
libs/designsystem/src/lib/directives/element-as-button/element-as-button.directive.spec.ts
Show resolved
Hide resolved
jkaltoft
requested changes
Mar 8, 2022
| @@ -32,6 +33,10 @@ | |||
| } | |||
| } | |||
| } | |||
| &.compact { | |||
| //TODO: verify if this is OK. It enables focus ring to not be cut off for first chip | |||
There was a problem hiding this comment.
What's the status on this TODO?
libs/designsystem/src/lib/components/slide-button/slide-button.component.scss
Show resolved
Hide resolved
libs/designsystem/src/lib/components/slide-button/slide-button.component.scss
Outdated
Show resolved
Hide resolved
| ion-toggle { | ||
| @include interaction-state.apply-focus-part($part: 'track'); | ||
| overflow: visible; | ||
| contain: none; |
There was a problem hiding this comment.
I'm not completely comfortable with CSS Containment, yet. Maybe it's just me being ignorant, but I'd appreciate a short comment on why we need the contain: none declaration.
And just for all intents and purposes, did you verify that browser adaptation of CSS Containment is ok?
There was a problem hiding this comment.
Added a comment to clarify this.
3bf5f49 to
9a823dc
Compare
|
@jkaltoft and I agreed today that we should merge this to the existing hover theme, so all this can be tested and released together. |
0c095e9
into
release/theme/2028-button-attention-levels-and-hover
jkaltoft
pushed a commit
that referenced
this pull request
Mar 14, 2022
* 💄 Apply focus-within to button * 💄 Fix checkbox border radius assignment * 💄 Apply focus-within to checkbox * 💄 Apply focus-within to radio * 💄 Apply focus-within to toggle * 💄 Change global focus ring color * 💄 Apply focus-within to card * 💄 Apply focus-within to dropdown * 💄 Apply focus-within to input (formfield) * 💄 Apply focus-within to textarea (formfield) * 💄 Apply focus-within to link * 💄 Apply focus-within to knob in range * 💄 Specify padding on segment btn (closes #1744) This is a prerequisite for properly styling the segment button with a focus ring * 💄 Apply focus-within to segmented-control * 💄 Use element-as-button directive with chip * 💄 Apply focus-within to chip * 🚧 Remove outline test from elm as btn directive This is part of restructuring focus ring tests. It is the responsibility of components themselves not this directive to apply focus styling. * 💄 Introduce apply-focus mixin * 💄 Use interaction state in button * 💄 Extend apply-focus mixin with gap and shadow * 💄 Use apply-focus in card * 💄 Use apply-focus in link * ⏪ Revert accidental color change * ♻️ Remove accidental double pseudo class for link * 💄 Adjust link focus ring border radius * 💄 Use background color getter from utils * 💄 Use apply-focus in dropdown * 💄 Split mixin in two to support css shadow parts * 💄 QoL improvements to focus mixin * 💄 Use apply-focus-part in toggle * ♻️ Clean up link, card and dropdown * 💄 Fix missing string interpolation * ♻️ Move apply-focus to shared form field styles * 💄 Use apply-focus in checkbox * 💄 Use apply-focus in chip * 💄 Use apply-focus in radio * 💄 Use apply-focus in range * 💄 Use apply-focus in segmented-control * 💄 Use apply-focus-part in Segmented Control * 💄 Support elevation of four in card * 💄 Add focus-visible mixin with fallbacks * 💄 Replace apply-focus with focus-visible solution * 💄 Apply mixins only on non-touch devices * ♻️ Refactor navbar to leave room for focus ring * 💄 Document and cleanup focus mixins * 🎨 Formatting to improve readability * Remove typo in focus mixin comment * ♻️ Add content-block to mixin * ♻️ Reorder imports alphabetically * ♻️ Use focus-ring-color variable via utils * 📝 Update comments for focus to be more specific * ♻️ Document toggle style changes * ♻️ Reorder for clarity * 🔥 Remove todo
jkaltoft
pushed a commit
that referenced
this pull request
Mar 17, 2022
* 💄 Apply focus-within to button * 💄 Fix checkbox border radius assignment * 💄 Apply focus-within to checkbox * 💄 Apply focus-within to radio * 💄 Apply focus-within to toggle * 💄 Change global focus ring color * 💄 Apply focus-within to card * 💄 Apply focus-within to dropdown * 💄 Apply focus-within to input (formfield) * 💄 Apply focus-within to textarea (formfield) * 💄 Apply focus-within to link * 💄 Apply focus-within to knob in range * 💄 Specify padding on segment btn (closes #1744) This is a prerequisite for properly styling the segment button with a focus ring * 💄 Apply focus-within to segmented-control * 💄 Use element-as-button directive with chip * 💄 Apply focus-within to chip * 🚧 Remove outline test from elm as btn directive This is part of restructuring focus ring tests. It is the responsibility of components themselves not this directive to apply focus styling. * 💄 Introduce apply-focus mixin * 💄 Use interaction state in button * 💄 Extend apply-focus mixin with gap and shadow * 💄 Use apply-focus in card * 💄 Use apply-focus in link * ⏪ Revert accidental color change * ♻️ Remove accidental double pseudo class for link * 💄 Adjust link focus ring border radius * 💄 Use background color getter from utils * 💄 Use apply-focus in dropdown * 💄 Split mixin in two to support css shadow parts * 💄 QoL improvements to focus mixin * 💄 Use apply-focus-part in toggle * ♻️ Clean up link, card and dropdown * 💄 Fix missing string interpolation * ♻️ Move apply-focus to shared form field styles * 💄 Use apply-focus in checkbox * 💄 Use apply-focus in chip * 💄 Use apply-focus in radio * 💄 Use apply-focus in range * 💄 Use apply-focus in segmented-control * 💄 Use apply-focus-part in Segmented Control * 💄 Support elevation of four in card * 💄 Add focus-visible mixin with fallbacks * 💄 Replace apply-focus with focus-visible solution * 💄 Apply mixins only on non-touch devices * ♻️ Refactor navbar to leave room for focus ring * 💄 Document and cleanup focus mixins * 🎨 Formatting to improve readability * Remove typo in focus mixin comment * ♻️ Add content-block to mixin * ♻️ Reorder imports alphabetically * ♻️ Use focus-ring-color variable via utils * 📝 Update comments for focus to be more specific * ♻️ Document toggle style changes * ♻️ Reorder for clarity * 🔥 Remove todo
9 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Which issue does this PR close?
This PR closes #1863 closes #1944 closes #1744
What is the new behavior?
Two focus mixins has been created and used across most interactive components.
The two mixins and their differences should hopefully be explained by the comments in _focus.scss.
Accordion-item, item and tabs has been left out for now as they are still being refined.
Does this PR introduce a breaking change?
Are there any additional context?
Checklist:
The following tasks should be carried out in sequence in order to follow the process of contributing correctly.
Reminders
Review
When the pull request has been approved it will be automatically merged to master via automerge.