GAUD-8850: Remove select.scss#7155
Conversation
- Adds feature flag to wrap the logic to always return the :focus-visible css pseudo-class
|
Thanks for the PR! 🎉 We've deployed an automatic preview for this PR - you can see your changes here:
Note The build needs to finish before your changes are deployed. |
EdwinACL831
force-pushed
the
ecollazos/GAUD-8850_remove_select_scss
branch
from
29eb27f to
21ea095
Compare
There was a problem hiding this comment.
I added back the code in the vdiffs just to make sure they are still looking as expected. Here is the report. this will be reverted once we are happy (get some thumbs up) with the changes and their results
There was a problem hiding this comment.
Actually, give me a bit longer to review this before reverting.
| outline: 2px solid var(--d2l-theme-border-color-focus); | ||
| outline-offset: -2px; | ||
| }`), | ||
| ariaInvalid: getFocusVisibleStyles(ariaInvalidSelector, (selector) => css`${selector} { |
There was a problem hiding this comment.
This rule is being used both inside and outside the media query, so just added it once
There was a problem hiding this comment.
I had to also update these styles in order to use the _getFocusVisibleStyles and replace the getFocusPseudoClass cause this file is imported in at the beginning of the input-select-styles.js and generating the styles in BSI would also had the same issue.
Note: This file is being imported in input-select-styles.js because the --d2l-input-invalid-image css variable is being used there, so we need to register it too.
EdwinACL831
force-pushed
the
ecollazos/GAUD-8850_remove_select_scss
branch
from
01f5b44 to
a1b7f07
Compare
| const ariaInvalidSelector = `${selector}[aria-invalid="true"], ${selector}[aria-invalid="true"]:hover, ${selector}[aria-invalid="true"]`; | ||
|
|
||
| return { | ||
| notDisabled: getFocusVisibleStyles(notDisabledSelector, (selector) => css`${selector} { |
There was a problem hiding this comment.
I think this block could just use getFocusRingStyles directly and pass in the box-shadow as extraStyles. That way we have one less duplication of the outline/outline-offset. Although then I think we'd lose the box-shadow: none in the high-contrast version... which I'm not sure how important that was.
There was a problem hiding this comment.
I could update that function as well to receive additional styles for the @media(prefer-contrast: more) query. Also will test this first locally and see how does that looks like
| </svg>` | ||
| ); | ||
|
|
||
| function _getSelectFocusStyles(selector) { |
There was a problem hiding this comment.
I'm personally finding the addition of this helper really confusing to follow the before & after, vs. just calling into getFocusVisibleStyles directly from _generateSelectStyles right inline.
There was a problem hiding this comment.
imo i think should be pretty straight forward. what I did was only to group styles and selectors into this function, and then use the returned object there. I decided to created this helper function to encapsulate the logic that replaces the getPseudoClass from above
EdwinACL831
force-pushed
the
ecollazos/GAUD-8850_remove_select_scss
branch
from
a172bca to
d9008b4
Compare
There was a problem hiding this comment.
We should add this (and the others that we missed) to node-imports-test.js.
EdwinACL831
force-pushed
the
ecollazos/GAUD-8850_remove_select_scss
branch
from
8979095 to
612d1f3
Compare
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
2 participants
Jira
GAUD-8850