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
Proposed resolution
Twig Templates to modify
- core/modules/system/templates/page.html.twig
- core/modules/system/templates/pager.html.twig
- core/modules/system/templates/progress-bar.html.twig
Remaining tasks
User interface changes
See parent issue(s) and related issue(s) for details,
Comment | File | Size | Author |
---|---|---|---|
#21 | p-templates-5.diff | 10.3 KB | valderama |
#20 | p-templates-4.diff | 9.61 KB | valderama |
#16 | p-templates-3.diff | 9.01 KB | mortendk |
#11 | progressbar-before.png | 42.52 KB | rteijeiro |
#11 | progressbar-after.png | 43.58 KB | rteijeiro |
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 commentedprogress-bar.html.twig
have a lot of tie ins withcore/misc/progress.js
must be looked into renaming classes there to js- progress__label etc
pager.html.twig & page.html.twig should be clean
Comment #5
mortendk CreditAttribution: mortendk commentedComment #6
mortendk CreditAttribution: mortendk commentedpregressbar css needs to be renamed & tested
Comment #7
Xen CreditAttribution: Xen commentedGrabbing.
Comment #8
Xen CreditAttribution: Xen commentedArg.. Ungrabbing.
Comment #9
mortendk CreditAttribution: mortendk commentedComment #10
mortendk CreditAttribution: mortendk commentedprefixed the js needed classes with .js- and took screenshots
Comment #11
rteijeiro CreditAttribution: rteijeiro commentedGreat job @mortendk!
Progressbar BEFORE
Progressbar AFTER
Comment #12
alexpottSomething is feeling really wrong about prefixing everything that javascript touches with js- especially when the class is being used to give the element style.
Comment #13
davidhernandezAgreeing with Alex. I don't think js- classes should show up in CSS files, and most of the changes in this patch are CSS. The point of the prefix is to highlight classes that are only needed for javascript. If we use them for both, it is no different than using the non-js classes for style and javascript. I'm thinking add separate js- classes or rework the javascript.
Comment #14
mortendk CreditAttribution: mortendk commentedyeah looks like we also have to rewrite all of that
Well now at least we know where this body is burried
Comment #15
mortendk CreditAttribution: mortendk commentedComment #16
mortendk CreditAttribution: mortendk commentedok i have taken same approach with the js- prefix as in forms & indentation etc.
js- prefix classes added in for js operations
Comment #17
alexpottWe need to update the CR
Comment #18
mortendk CreditAttribution: mortendk commentedupdated with the changes in the css classes, whats added in as js- prefixes & which classes we have removed in core, but is in classy
Do we want to fix the css MAT seperation here as well - and screenshots since we are changing js selectors ?
Comment #19
dawehnerMaybe it would be nice to do #2452381: Use Drupal.theme for progress.js
So, I see
in core/modules/views/js/ajax_view.js:105 ... doesn't this break things?
Comment #20
valderama CreditAttribution: valderama commentedHi - found a reference to this issue on twitter, and thought I could do a quick patch.
I have taken on dawehners second point from the last comment and added a class "js-pager__items" to the ul-tag - and changed the jQuery selector in ajax_views.js accordingly. That should keep this part working. Everything else is the same as in the last patch.
And a little side note: Keeping the empty div here seems strong. Maybe a more semantic tag would be better? It is not a big deal however...
Comment #21
valderama CreditAttribution: valderama commentedI forgot to add the "js-pager__items" class also to the classy version of pager.html.twig. Included in the new patch.
I have tested the ajax pager now in bartik, seven and stark - and it works now in classy-themes (bartik and seven) and in non-classy themes (stark).
Comment #23
valderama CreditAttribution: valderama commentedI guess the PagerTest fails because it looks for li.pager__items - which are not present in non-classy themes. Same for AggregatorRenderingTest.
So the patch fails because simpletest uses a non-classy theme?
Comment #24
mortendk CreditAttribution: mortendk as a volunteer commentedbumb
Comment #37
quietone CreditAttribution: quietone at PreviousNext commented