-
Notifications
You must be signed in to change notification settings - Fork 11
refactor: CSS "condition" to SCSS condition #4690
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
base: main
Are you sure you want to change the base?
Conversation
🔭🐙🐈 Test this branch here: https://db-ux-design-system.github.io/core-web/review/refactor-css-condition-to-scss-condition |
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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.
Pull Request Overview
This refactoring moves CSS element type checking from runtime CSS :is()
selectors to build-time SCSS conditions, improving maintainability and performance by reducing CSS complexity. The change replaces CSS pseudo-selectors with SCSS conditional compilation.
- Replaced CSS
:is(input, textarea)
selectors with SCSS@if
conditions usinglist.index()
- Added SCSS variables to define valid form field types for read-only and field-sizing properties
- Updated CSS variable naming to be dynamic based on the element type
// https://developer.mozilla.org/en-US/docs/Web/CSS/:read-only | ||
$valid-read-only-form-fields: input, textarea; | ||
// https://developer.mozilla.org/en-US/docs/Web/CSS/field-sizing | ||
$valid-field-sizing-form-fields: input, textarea; | ||
|
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.
Currently, both $valid-field-sizing-form-fields and $valid-read-only-form-fields contain the same elements (input, textarea). Do we need to keep them separate for semantic clarity, or could we merge them into a single list to reduce redundancy?
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 was struggling with this as well. Perhaps there is no clear right or wrong, but when it comes to choosing between DRY and distinguishing between two completely different CSS features, the latter may be slightly more relevant.
@if list.index($valid-read-only-form-fields, $selector) { | ||
&:not(:disabled):read-only { | ||
background-color: var( | ||
--db-#{$selector}-read-only, |
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 was --db-textarea-read-only before and is now element-specific. I think that is fine.
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 was --db-textarea-read-only before and is now element-specific. I think that is fine.
In general this isn't a variable we're providing, but only something we provide as a "Styling API". So scoping this to the "correct" component (input
instead of textarea
) might even be benefical.
Proposed changes
Currently we've checked for the elements to be either
input
ortextarea
at the CSS level with:is()
. Instead as we're using those in a SCSS mixin, we should that for check for those element types in a SCSS condition.The main purpose of this is to prevent resulting selectors like
.db-input input:is(input,textarea):not(:disabled):read-only
Types of changes
Further comments