Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
Comment | File | Size | Author |
---|---|---|---|
#10 | drupal8-split-toolbar.js-2162407-10.patch | 38.96 KB | jibran |
#3 | drupal8-split-toolbar.js-2162407-3.patch | 38.87 KB | jibran |
Comments
Comment #1
nod_Comment #2
nod_Comment #3
jibranHere we go.
Comment #4
jibranI haven't seen this library used in toolbar.js can we remove this or am I missing something.
Comment #5
nod_probably the CSS for the SVG or something.
Comment #6
nod_Perfect, thank you. Much, much better than one file with 700+ lines.
Comment #7
xjm3: drupal8-split-toolbar.js-2162407-3.patch queued for re-testing.
Comment #8
nod_3: drupal8-split-toolbar.js-2162407-3.patch queued for re-testing.
Comment #10
jibranreroll
Comment #11
nod_All good, thanks :)
Comment #12
webchickI think this makes sense, but letting Jesse Beach chime in here to confirm.
Comment #13
jessebeach CreditAttribution: jessebeach commentedThe 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.
Comment #14
jessebeach CreditAttribution: jessebeach commentedBack to webchick.
Comment #15
webchickGreat, thanks!
Committed and pushed to 8.x. We should probably backport this to D7 too to make future maintenance easier.
Comment #16
eshta CreditAttribution: eshta commentedI'm starting on this and will also utilize this issue to sync up our javascript with the D8 toolbar - better late than never ;-)
Comment #17
eshta CreditAttribution: eshta at Acquia commentedThis 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.
Comment #18
hass CreditAttribution: hass commentedI vote for upgrading underscore, too.