Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Problem/Motivation
Remove non essential classes from core - the classes are in classy, cores templates should be as clean as possible.
js required classes should be prefixed to js- according to code standards
simpletest should be done with classy theme, not stark
See parent issue(s) and related issue(s) for details,
Proposed resolution
Twig Templates to modify
- core/modules/system/templates/radios.html.twig
- core/modules/system/templates/region.html.twig
Remaining tasks
User interface changes
API changes
Comment | File | Size | Author |
---|---|---|---|
#36 | templates-r-fix-quickedit-36.diff | 4 KB | mortendk |
#34 | templates-r-fix-quickedit.diff | 4.03 KB | mortendk |
#34 | stark.png | 321.24 KB | mortendk |
#34 | bartik.png | 401.91 KB | mortendk |
#34 | seven.png | 338.59 KB | mortendk |
Comments
Comment #1
Sivaji_Ganesh_Jojodae CreditAttribution: Sivaji_Ganesh_Jojodae commentedComment #2
davidhernandezPostponing this until we decide how we are moving forward. Keep an eye on #2348543: [meta] Consensus Banana Phase 2, transition templates to the starterkit theme for updates. Thanks.
Comment #3
davidhernandezUn-postponing. The templates have been copied to Classy, so all we need to do is remove classes from the original templates.
Comment #4
mortendk CreditAttribution: mortendk commentedComment #6
mortendk CreditAttribution: mortendk commentedComment #7
mortendk CreditAttribution: mortendk commentedlets try this again
Comment #8
davidhernandezThe only CSS I see attached to form-radios is margin. I don't think I see anything using 'region' directly but I do see some JS using region-*, do we need to change those? For example, quickedit looks for region-content.
Comment #9
mortendk CreditAttribution: mortendk commented@david sounds like we should do a cleanup on quickedit as a seperate issue, or eventually scopecreep this ?
Comment #10
davidhernandezWell, I don't think we can commit anything here without accounting for it, otherwise we introduce a regression.
Comment #11
mortendk CreditAttribution: mortendk commentedComment #12
mortendk CreditAttribution: mortendk commentedComment #13
mortendk CreditAttribution: mortendk commentedI would suggest that we then do a css standards on quicklinks cleanup & then at the same time there fixes any calls to region-content.
- yes more followups i know, but :/
Comment #14
mortendk CreditAttribution: mortendk commentedDiggin into this:
Could somebody point to the place that says that drupal have to have a "content" region - which is ok to assume.
But if that region is there then we assume that offcourse we have a wrapper around it thats called
.region-content
t around the regionIt seems like were requiring themes to have this, if thats the case - Then it should be documented somewhere & the classnames must indicate this as well in this case with a js- prefix - If this is not the case we should clean this up.
Comment #15
davidhernandezhttps://www.drupal.org/node/171224
I knew this was true for 7, but have never checked to see if it was a hard requirement for 8.
Comment #16
mortendk CreditAttribution: mortendk commentedok cleaned it up a bit and removed the .region-content selector, as we cant rely on that whatsoever to be a part of a theme (usually one of the first places anybody goes to cleanup - when the div soup is to thick) I changed the selected from
region-content
to bemain
instead.We should use
<main>
instead of.region-content
- Based on the following reasons:* using css for this is terrible - ill keep the rant down! but .... *grrr*
* Main should always be present on a page, its like having a head / footer so it makes perfect sense.
* Main is an unique tag that there should be only once on a page (https://developer.mozilla.org/en-US/docs/Web/HTML/Element/main) - hurray without using an ID
This should be documented somewhere in the big book of "what goes on in drupals frontend", but maybe in the page.html.twig as well ?
Comment #17
davidhernandezShould change that comment about using "content" region, since it is being changed to
<main>
.Also, this needs some testing to make sure it is still working right, whatever it does. This entityElement, it is being moved from the region-content div to the main tag which is two levels up in the markup.
Comment #18
mortendk CreditAttribution: mortendk commentedSelector changed to be
main[role="main"]
which is the default wrapper for "content" and the full view of a nodeIn that way we dont "sneak in" a dependency on
.region-content
that nobody have an idea about and<main role="main">
is always present.Comment #19
mortendk CreditAttribution: mortendk commentedComment #20
rteijeiro CreditAttribution: rteijeiro commentedFixed a typo in a comment.
Comment #21
rteijeiro CreditAttribution: rteijeiro commentedAttached a few screenshots:
Radios BEFORE
Radios AFTER
QuickEdit BEFORE
QuickEdit AFTER
Comment #22
mortendk CreditAttribution: mortendk commentedCR updated as well
Comment #23
alexpottLooks like this can be removed.
Comment #24
lduerig CreditAttribution: lduerig commentednew patch
Comment #25
lduerig CreditAttribution: lduerig commentedComment #26
lduerig CreditAttribution: lduerig commentedComment #27
rteijeiro CreditAttribution: rteijeiro commentedWrong indentation.
Comment #29
davidhernandezJust fixing that indent.
For this change, I'd be real careful about checking all the region stuff. That's what worries me in possibly introducing a regression. Make sure to test with Stark.
Comment #30
mortendk CreditAttribution: mortendk as a volunteer commented@david
We change the selector from using the class
.region-content
thats only in the region thats named "content" & move that up to<main role=" main">
thats unique.im kinda thinking that we maybe should add a data-attibute, or adding in a class lik "js-quickedit-selector-foo" to make it more robust + needs comments in the source so its clear that we use this as a selector for quickedit
But yes needs test anyways
Comment #31
davidhernandezI'm no JS wiz, but this seems to maintain the quickedit functionality.
Comment #32
joelpittetComment #33
mortendk CreditAttribution: mortendk as a volunteer commentedwe should really use a
js-quickedit
class here else nobody have an idea of whats going onComment #34
mortendk CreditAttribution: mortendk as a volunteer commented@david think one of the win we could get with removing the quickedit dependency from the region template, we could make it more obvious where it comes from and whats happening in the code. In that light i would suggest that we put it out in the main region and used
.js-quickselect
to make it clear for the themer whats going on + we dont fill all the region divs with data-- that will probably scare a lot of people.a reroll + manual testing of all existing themes.
Comment #35
LewisNyman CreditAttribution: LewisNyman at Wunder commentedI think this is a good idea. Can we change this class to something more meaningful? This isn't a trigger for all of quickedit, this is a specific selector for the main content container, how about
.js-quickedit-main-content
? Also there is whitespace at the end of this line.Comment #36
mortendk CreditAttribution: mortendk as a volunteer commented@lewis good point ive updated the patch
Comment #37
LewisNyman CreditAttribution: LewisNyman at Wunder commentedI manually tested this and I'm happy with the changes here. I wave the RTBC wand.
Comment #38
alexpottMarkup is not frozen yet. Committed 22cd7b8 and pushed to 8.0.x. Thanks!
Comment #40
tstoecklerWow, it's very strange to have a reference to
quickedit
in System module's page template.I can see 2 ways to fix this (IMO) "properly".
1. Decide that finding the main-content is useful for all kinds of JS and call this "js-main-content"
2. Have Quickedit module add that in preprocess (this would require the addition of a variable for that I guess)
Comment #41
LewisNyman CreditAttribution: LewisNyman at Wunder commentedI guess this makes the most sense. This is why we allow modules to inject classes.
Comment #42
davidhernandezI think we'd need an attribute object for that. 'page' doesn't currently have one for any of the other elements. I wonder if that is just a coincidence or there is a reason.
Comment #43
mortendk CreditAttribution: mortendk as a volunteer commentedits even stranger to have the selector hidden inside a regions classes with no references to it ;)
Seriously lets not have preprocess add in all kinds of stuff, just so we can hide it, its been an epic pita for years, that so much have been hidden.
yes we allow module to add in classes, but really they should not do that, its a fallback for when its not possible otherwise, its keeping us in the "preprocess for everything" thinking of drupal7 - lets get it out in the open so we can see & look at it, yes this is TX to the max.
Adding in {{ attributes }} ... hmmm yes but we have em all over and nobody knows what they are used for, they are just there, so unless theres a really good reason for that, lets not confuse ourself with this just to create anther box where we maybe can dump stuff into.
Comment #44
Wim LeersI just discovered this by accident, I was also very surprised by this.
What does "full entity ancestor" even mean? I wish you'd have pinged a Quick Edit maintainer for review.
Huh? By that reasoning, certain modules would never be able to function. They need to inject classes/markup in order to provide certain functionality.
The problem is that we can rely neither on:
<main>
being present (not every theme may have this)[role=main]
being present (not every theme may have this).page-title
being present (some themes may just use<h1>
)Quick Edit just needs to know the DOM node in which the other fields of the entity besides the title live. Perhaps there's another way possible now, compared to 3 years ago?
Comment #45
mortendk CreditAttribution: mortendk as a volunteer commentedYes certain modules may need to do this, but we should work hard to have all classes inside of the theme instead of hidden a few away in a module.
It makes it hard to find for a themer & makes it a pita to track where stuff comes from.
It should always be the exception to add a class from a module.
main ftw
actually we can rely on themes having the main tag, the same way as we can rely on them having a body or footer.
<main>
should always be present in a page - not having a role="main" would also go against the accesabilityhttps://developer.mozilla.org/en/docs/Web/HTML/Element/main
.page-title yes please let us remove that, its one of these.
Comment #46
Wim LeersPerfect! In that case, Quick Edit can just rely on that! Much simpler. None of the ugliness introduced here is necessary anymore then.
Patch at: #2568099: Follow-up for #2407739: Remove the js-quickedit-main-content class that was added in favor of relying on <main>. Let's clean this up :)
Comment #48
Wim Leers