(First, verify that the preprocess changes have been made. #2322163: [meta] Consensus Banana Phase 1, move CSS classes from preprocess to twig templates.)

  1. Copy the Twig templates from the core module's templates directory to Classy's templates directory. Include all templates, even ones without classes.
  2. Remove all classes from the core module's template. Remove all classes added with addClass and ones that are hard-coded in the template.
  • If there are classes that are required for basic functionality, discuss whether they should be kept.
  • If there is CSS from the module, or anywhere else, referring to the class, discuss removing it or moving it to Bartik&Seven. Do not move the CSS to Classy.

Twig Templates to Copy

core/modules/toolbar/templates/toolbar.html.twig

CommentFileSizeAuthor
#20 copy_toolbar_templates-2349767-20.patch1.37 KBmortendk
#17 Screen Shot 2015-02-03 at 11.51.02 .png88.55 KBmortendk
#17 Screen Shot 2015-02-03 at 11.51.10 .png86.2 KBmortendk
#13 copy-toolbar-templates-2349767-11.patch1.43 KBsaki007ster
#6 copy-toolbar-templates-2349767-6.patch40.63 KBkallehauge
#6 interdiff.txt40.09 KBkallehauge
#6 stark2.png100.14 KBkallehauge
#1 copy_toolbar_templates-2349767-1.patch3.07 KBlauriii

Comments

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Active » Needs review
StatusFileSize
new copy_toolbar_templates-2349767-1.patch3.07 KB

Status: Needs review » Needs work

The last submitted patch, 1: copy_toolbar_templates-2349767-1.patch, failed testing.

mortendk’s picture

mortendk commented
Issue tags: +drupalhagen
davidhernandez’s picture

davidhernandez
Primary language English
Location USA
commented

Please double-check if any removed classes are being used in javascript. It is best to test the affected template using Stark to make sure nothing is broken.

kallehauge’s picture

kallehauge commented
Assigned: Unassigned » kallehauge

Going to work

kallehauge’s picture

kallehauge commented
Assigned: kallehauge » Unassigned
Status: Needs work » Needs review
StatusFileSize
new stark2.png100.14 KB
new interdiff.txt40.09 KB
new copy-toolbar-templates-2349767-6.patch40.63 KB

I think I got it all now. I have renamed all the classes used by JavaScript to begin with "js-..." and changed all the places where any CSS/JS reference the classes.

The two templates inside Classy and the toolbar module are now identical again. If we removed the "visually-hidden" classes, then the toolbar won't look as intended when Stark is enabled. - I also left in the "clearfix" classes for good measures since I don't know if they're essential for cross-browser support. I've only tested it in Chrome.

Look at the attached image to see an example of how it "breaks" without the necessary visibility classes that was previously removed.

mortendk’s picture

mortendk commented
Status: Needs review » Needs work

looks like the vertical orientation for the toolbar somehow got removed, setting it back to needs work

DickJohnson’s picture

DickJohnson commented
Assigned: Unassigned » DickJohnson

Re-assigned. Trying to get a patch online in 24 hours.

DickJohnson’s picture

DickJohnson commented
Assigned: DickJohnson » Unassigned

Status: Needs work » Needs review

lauriii queued 6: copy-toolbar-templates-2349767-6.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 6: copy-toolbar-templates-2349767-6.patch, failed testing.

saki007ster’s picture

saki007ster
he/his
Location Buffalo, New York
commented
Assigned: Unassigned » saki007ster
saki007ster’s picture

saki007ster
he/his
Location Buffalo, New York
commented
StatusFileSize
new copy-toolbar-templates-2349767-11.patch1.43 KB

Copied the template from the toolbar module to the classy theme and took care of the classes which should remain in the file.

saki007ster’s picture

saki007ster
he/his
Location Buffalo, New York
commented
Status: Needs work » Needs review
saki007ster’s picture

saki007ster
he/his
Location Buffalo, New York
commented
Assigned: saki007ster » Unassigned
mortendk’s picture

mortendk commented
Issue tags: -drupalhagen

create a followup issue to fix the lacking of js prefix for toolbar's classes #2349767: Copy toolbar templates to Classy

mortendk’s picture

mortendk commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new Screen Shot 2015-02-03 at 11.51.10 .png86.2 KB
new Screen Shot 2015-02-03 at 11.51.02 .png88.55 KB

Looks good

classes should be modified in the followup issue its not the role of this patch to fix those issues

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/modules/toolbar/templates/toolbar.html.twig
@@ -33,10 +33,10 @@
-          <h3 class="toolbar-tray-name visually-hidden">{{ tray.label }}</h3>

Removing the toolbar-tray-name breaks accessibility - see core/modules/toolbar/js/views/ToolbarAuralView.js

mortendk’s picture

mortendk commented

ooh my yes we do need indeed to prefix these classes, maybe thats even in this issue

mortendk’s picture

mortendk commented
Status: Needs work » Needs review
StatusFileSize
new copy_toolbar_templates-2349767-20.patch1.37 KB

added backin the toolbar tray-name updated the followup issue on proper css classnaming when depending on js

davidhernandez’s picture

davidhernandez
Primary language English
Location USA
commented
Status: Needs review » Reviewed & tested by the community

Seems like we need to keep most of those classes for functionality. Nice to get rid of the clearfix though.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 20: copy_toolbar_templates-2349767-20.patch, failed testing.

Status: Needs work » Needs review

mortendk queued 20: copy_toolbar_templates-2349767-20.patch for re-testing.

davidhernandez’s picture

davidhernandez
Primary language English
Location USA
commented
Status: Needs review » Reviewed & tested by the community

The failed test was broken HEAD. We're back in action now.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.0.x. Thanks!

  • alexpott committed 1a18f03 on 8.0.x 
    Issue #2349767 by mortendk, kallehauge, saki007ster, lauriii: Copy...
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

I pushed on @webchick's behalf :)

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

Whew! Thanks. :)

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.