Problem/Motivation

toolbar is a beast css classes are used allover as js selectors & for visua styling it all makes it close to impossible to work with it :(
theres already 3-4 other issues that are trying to cleanup / simply toolbar, This work will be hard to get done prober if a seperation isnt done first, and as a part of the banana concensus we want to seperate css from javascript selectors.

Proposed resolution

add in prefixed .js- classes to seperate the css & js
same approach as done with forms & indentation.

Remaining tasks

User interface changes

API changes

Files: 
CommentFileSizeAuthor
#26 interdiff-2452343-24-26.txt698 byteshimanshu-dixit
#26 2452343-26.patch12.39 KBhimanshu-dixit
#24 toolbar-2452343-23.patch12.4 KBsudhanshug
#18 toolbar-2452343-18.patch16.01 KBpektinasen
#6 toolbar-3.diff15.53 KBmortendk
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,573 pass(es). View
#4 toolbar-2.diff15.92 KBmortendk
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch toolbar-2.diff. Unable to apply patch. See the log in the details link for more information. View
toolbar.diff16.83 KBmortendk
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,789 pass(es). View

Comments

mortendk’s picture

Issue summary: View changes
LewisNyman’s picture

Status: Needs review » Needs work
+++ b/core/modules/toolbar/css/toolbar.module.css
@@ -231,12 +231,12 @@ body.toolbar-tray-open.toolbar-vertical.toolbar-fixed {
-.toolbar .toolbar-tray .toolbar-toggle-orientation {
+.toolbar .toolbar-tray .js-toolbar-toggle-orientation {
...
-.toolbar-oriented .toolbar-tray .toolbar-toggle-orientation {
+.toolbar-oriented .toolbar-tray .js-toolbar-toggle-orientation {

This is not correct. We shouldn't have js- classes used for styling in our CSS. We should only use use js- classes for javascript functionality.

mortendk’s picture

Status: Needs work » Needs review
FileSize
15.92 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch toolbar-2.diff. Unable to apply patch. See the log in the details link for more information. View

removed the js- css stuff

Status: Needs review » Needs work

The last submitted patch, 4: toolbar-2.diff, failed testing.

mortendk’s picture

Status: Needs work » Needs review
FileSize
15.53 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,573 pass(es). View

rerolled

brahmjeet789’s picture

FileSize
23.19 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch toolbar_add_javascript_2452343-7.patch. Unable to apply patch. See the log in the details link for more information. View

Status: Needs review » Needs work

The last submitted patch, 7: toolbar_add_javascript_2452343-7.patch, failed testing.

brahmjeet789’s picture

FileSize
13.17 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch toolbar_add_javascript_2452343-8.patch. Unable to apply patch. See the log in the details link for more information. View
pjbaert’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 9: toolbar_add_javascript_2452343-8.patch, failed testing.

MathieuSpil’s picture

@brahmjeet789 I tried a interdiff of #6 and #8 and I noticed we going a total different way.

I would suggest we build a bit more on @mortendk's latest suggestion.

Patch from #6 no longer applies, so we need a reroll here.

MathieuSpil’s picture

MathieuSpil’s picture

Assigned: Unassigned » MathieuSpil
MathieuSpil’s picture

@pektinasen will create this reroll.

After turning that patch green, we will merge this ticket with #2419135: make toolbar module follow the css documentation because of exactly the same code almost.

MathieuSpil’s picture

Assigned: MathieuSpil » Unassigned
pektinasen’s picture

Assigned: Unassigned » pektinasen
pektinasen’s picture

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

sudhanshug’s picture

Status: Needs work » Needs review

Changing status to test the patch.

Status: Needs review » Needs work

The last submitted patch, 18: toolbar-2452343-18.patch, failed testing.

sudhanshug’s picture

Status: Needs work » Needs review
FileSize
12.4 KB

Re-rolled the patch from 18 to apply cleanly to the latest D8

Status: Needs review » Needs work

The last submitted patch, 24: toolbar-2452343-23.patch, failed testing.

himanshu-dixit’s picture

Status: Needs work » Needs review
FileSize
12.39 KB
698 bytes

Looks like testbot doesn't agree to the last patch. So, I have removed quotes from the property since it is redundant according to coding standards.

Status: Needs review » Needs work

The last submitted patch, 26: 2452343-26.patch, failed testing.