Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Issue #2473955 by mortendk, rteijeiro, rachel_norfolk, Cottser, LewisNyman: Prefix form-wrapper classes with js-
Task
Prefix form-wrapper classes with js- to separate classes needed for JavaScript functionality from those used for styling and make it clear which classes can safely be removed without breaking JavaScript functionality.
Remaining tasks
- Patch
- Patch review
- Manual testing
Steps to test
@todo
Beta phase evaluation
Issue category | Task |
---|---|
Issue priority | Normal |
Unfrozen changes | Unfrozen because it mostly just affects templates and JS which are not frozen. |
Prioritized changes | The main goal of this issue is to improve themer experience and arguably it reduces fragility where JavaScript and markup intersect. |
Disruption | Shouldn't be too disruptive as it is mostly affecting CSS classes that are added to markup. Themes extending Classy will only have classes added. Themes not extending Classy will have classes removed but they can be added back via template overrides. |
User interface changes
None for themes extending Classy. Possible visual changes for other themes.
API changes
n/a
Comment | File | Size | Author |
---|---|---|---|
#11 | interdiff.txt | 847 bytes | star-szr |
#11 | prefix_form_wrapper-2473955-11.patch | 6 KB | star-szr |
#10 | prefix_form_wrapper-2473955-10.patch | 5.94 KB | star-szr |
#1 | 2473955-form-wrapper-split.patch | 5.93 KB | star-szr |
Comments
Comment #1
star-szrPatch split from parent issue. I wasn't able to find anything additional by grepping.
Comment #2
LewisNyman CreditAttribution: LewisNyman at Wunder commentedI grepped in the core directory and couldn't find any other changes. I couldn't figure out where to test this change though.
I think that we should remove the non-functional classes from the tests, what do you think?
Comment #3
star-szrAdding suggested commit message.
Comment #4
LewisNyman CreditAttribution: LewisNyman at Wunder commentedWe had this discussion in other issues, we shouldn't be updating the tests here and instead improve the tests in followup issues.
Comment #5
alexpottShouldn't we by having both form-wrapper and js-form-wrapper? Like we did in core/lib/Drupal/Core/Render/Element/Details.php?
Comment #6
star-szrWe don't add those classes to core markup, just Classy. When we don't have a choice (added via modules/JS), we add both.
Edited to use the right word for modules :)
Comment #7
LewisNyman CreditAttribution: LewisNyman at Wunder commentedThat makes sense for now, hopefully we can move where we are adding the classes in the future.
Comment #8
alexpottNeeds a reroll.
Comment #9
alexpottI'm not convinced by #6. What about
in locale.admin.css?
Comment #10
star-szrThanks, rerolled. I will try to catch @alexpott in IRC to discuss further :)
Comment #11
star-szrThere are quite a few things in core modules using this form-wrapper class so it does seem a bit rash to rip it out.
We can create a followup to discuss the CSS/form-wrapper class further, probably most of the styling could be moved to Classy at which point we could also remove the form-wrapper classes from the core templates. I don't have time right now to create the followup but here's a new patch.
As @alexpott pointed out, the most important thing is getting the js- classes in, not getting the non-js- classes out.
Comment #13
davidhernandezThis looks good to me. Covers all cases where I can find form-wrapper. I sent it for retesting to make sure it is still ok.
Comment #14
davidhernandezComment #15
alexpottCommitted 9a7a4c2 and pushed to 8.0.x. Thanks!
Thanks for adding the beta evaluation to the issue summary.