(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

Files: 
CommentFileSizeAuthor
#20 copy_toolbar_templates-2349767-20.patch1.37 KBmortendk
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,940 pass(es). View
#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
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,566 pass(es). View
#6 copy-toolbar-templates-2349767-6.patch40.63 KBkallehauge
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch copy-toolbar-templates-2349767-6.patch. Unable to apply patch. See the log in the details link for more information. View
#6 interdiff.txt40.09 KBkallehauge
#6 stark2.png100.14 KBkallehauge
#1 copy_toolbar_templates-2349767-1.patch3.07 KBlauriii
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,987 pass(es), 3 fail(s), and 0 exception(s). View

Comments

lauriii’s picture

Status: Active » Needs review
FileSize
3.07 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,987 pass(es), 3 fail(s), and 0 exception(s). View

Status: Needs review » Needs work

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

mortendk’s picture

Issue tags: +drupalhagen
davidhernandez’s picture

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

Assigned: Unassigned » kallehauge

Going to work

kallehauge’s picture

Assigned: kallehauge » Unassigned
Status: Needs work » Needs review
FileSize
100.14 KB
40.09 KB
40.63 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch copy-toolbar-templates-2349767-6.patch. Unable to apply patch. See the log in the details link for more information. View

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

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

Assigned: Unassigned » DickJohnson

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

DickJohnson’s picture

Assigned: DickJohnson » Unassigned

Status: Needs work » Needs review

Status: Needs review » Needs work

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

saki007ster’s picture

Assigned: Unassigned » saki007ster
saki007ster’s picture

FileSize
1.43 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,566 pass(es). View

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

Status: Needs work » Needs review
saki007ster’s picture

Assigned: saki007ster » Unassigned
mortendk’s picture

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

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
FileSize
86.2 KB
88.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

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

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

mortendk’s picture

Status: Needs work » Needs review
FileSize
1.37 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,940 pass(es). View

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

davidhernandez’s picture

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
davidhernandez’s picture

Status: Needs review » Reviewed & tested by the community

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

webchick’s picture

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

I pushed on @webchick's behalf :)

webchick’s picture

Whew! Thanks. :)

Status: Fixed » Closed (fixed)

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