-
Notifications
You must be signed in to change notification settings - Fork 6
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
Make required asterisk theme-independant #640
Make required asterisk theme-independant #640
Conversation
@include anchor.extend-utrecht-link; | ||
// TODO: remove font-weight rule on 2.0, it's only here for backwards compatibility | ||
font-weight: var(--utrecht-form-label-font-weight, var(--of-label-font-weight)); | ||
.utrecht-form-label { |
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 will also affect utrecht-form-label
components outside of our SDK root, so if a CMS solution has other forms on the page (e.g. contact), our CSS overrides leak outside of our own "zone" when embedding forms in pages.
To avoid this, I think we need to gate the CSS overrides like display
, line-height
etc. behind design tokens and define these design token values in our own openforms-theme
.
OR we scope these CSS overrides to .utrecht-form-label.utrecht-form-label--openforms
instead - that's probably the neater solution:
.utrecht-form-label {
@include bem.modifier('openforms') {
display: block;
line-height: 1.333;
overflow-wrap: break-word;
...
}
@include bem.modifier('openforms-required') { ... }
}
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.
Ah good one.
When the field is required the label does not get the .utrecht-form-label--openforms
class, only .utrecht-form-label--openforms-required
. What if we scope it to the generic openforms container?
.openforms-container {
.utrecht-form-label {
...
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.
.openforms-container
is not a class/component we have 🤔
When the field is required the label does not get the
.utrecht-form-label--openforms class
Yeah I think we should patch that extra utrecht-form-label--openforms
classname in (to always have it):
- https://github.com/open-formulieren/open-forms-sdk/blob/main/src/components/forms/Label.js#L15
- https://github.com/open-formulieren/open-forms-sdk/blob/main/src/formio/templates/label.ejs#L3
Not sure if there are any extra places where this would apply, but I think our visual regression testing will catch them if I missed anything.
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.
ah yes it's the class that formio wraps around the form I think. I like the approach you mentioned (instead of catching all scenario's with just css). Found a few other files where I added the "base" classname. Updated the PR!
Thanks for the contribution! |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #640 +/- ##
==========================================
- Coverage 74.85% 74.10% -0.75%
==========================================
Files 224 224
Lines 4542 4542
Branches 1211 1211
==========================================
- Hits 3400 3366 -34
- Misses 1103 1137 +34
Partials 39 39 ☔ View full report in Codecov by Sentry. |
… can be specified properly
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.
Very nice!
In _formio_component.scss the field-required asterisk is removed because it is handled with the "utrecht-form-label--openforms-required" class. But this class was scoped on the "openforms-theme", meaning it did not work with other themes.
As the showing/hiding of the asterisk is a core functionality I think this css should be scoped on all themes. Styling can be further tweaked in the specific design system.