The toolbar.js file needs to be split up. It's confusing to read. We use backbone to separate and organize our JS, it's a shame to stick everything in one file in the end (same goes for contextual module, separate issue will be filled).

The edit module has it's thing down, can I haz?:

js/toolbar.js
js/views/BodyVisualView.js
js/views/MenuVisualView.js
js/views/ToolbarAuralView.js
js/views/ToolbarVisualView.js
js/models/MenuModel.js
js/models/ToolbarModel.js
CommentFileSizeAuthor
#10 drupal8-split-toolbar.js-2162407-10.patch38.96 KBjibran
#3 drupal8-split-toolbar.js-2162407-3.patch38.87 KBjibran
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented
Title: Split up the JS file » Split up toolbar.js
nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented
Issue tags: +JavaScript clean-up
jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented
Status: Active » Needs review
Related issues: +#2162409: Split up contextual.js
FileSize
drupal8-split-toolbar.js-2162407-3.patch38.87 KB

Here we go.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented 
+++ b/core/modules/toolbar/toolbar.module
@@ -562,16 +562,26 @@ function toolbar_get_rendered_subtrees() {
       array('system', 'modernizr'),

I haven't seen this library used in toolbar.js can we remove this or am I missing something.

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented

probably the CSS for the SVG or something.

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented
Status: Needs review » Reviewed & tested by the community

Perfect, thank you. Much, much better than one file with 700+ lines.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented

3: drupal8-split-toolbar.js-2162407-3.patch queued for re-testing.

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented

3: drupal8-split-toolbar.js-2162407-3.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 3: drupal8-split-toolbar.js-2162407-3.patch, failed testing.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented
Status: Needs work » Needs review
FileSize
drupal8-split-toolbar.js-2162407-10.patch38.96 KB

reroll

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented
Status: Needs review » Reviewed & tested by the community

All good, thanks :)

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Assigned: Unassigned » jessebeach

I think this makes sense, but letting Jesse Beach chime in here to confirm.

jessebeach’s picture

jessebeach CreditAttribution: jessebeach commented

The test coverage for Toolbar is quite good. I trust the bot here.

Manual testing reveals no behavior regressions.

The code looks good. Great job jibran. This will make it easier for module developers to experiment with alternative JS if they so wish.

Let's commit it.

jessebeach’s picture

jessebeach CreditAttribution: jessebeach commented
Assigned: jessebeach » webchick

Back to webchick.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Project: Drupal core » Navbar
Version: 8.x-dev » 7.x-1.x-dev
Component: toolbar.module » Code
Assigned: webchick » Unassigned
Status: Reviewed & tested by the community » Patch (to be ported)

Great, thanks!

Committed and pushed to 8.x. We should probably backport this to D7 too to make future maintenance easier.

eshta’s picture

eshta CreditAttribution: eshta commented
Assigned: Unassigned » eshta
Status: Patch (to be ported) » Active

I'm starting on this and will also utilize this issue to sync up our javascript with the D8 toolbar - better late than never ;-)

eshta’s picture

eshta CreditAttribution: eshta at Acquia commented

This is going to require upgrading the requirements for navbar to include underscore 1.8.3 due to a fix in the deep equals comparison. Otherwise we'll need to make navbar work differently than toolbar.

Toolbar keeps a reference to the activeTab in the model and updates the tray shown based on changes to the activeTab in the model (makes sense). Navbar had been storing the id - a nice easy string. Unfortunately - underscore has a bug that will find two HTMLAnchorElement objects to be equal regardless of them having different values for their properties. As a result, changes to the activeTab in the model are not properly detected and the trays do not update.

We could continue to keep the code out of sync, but I see no reason to. Since underscore and backbone aren't parts of stock Drupal in version 7 there is no reason to be locked into an older version.

hass’s picture

hass CreditAttribution: hass commented

I vote for upgrading underscore, too.