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.In @Wim Leers' review in #164 of #2753941: [Experimental] Create Outside In module MVP to provide block configuration in Off-Canvas tray and expand site edit mode
All of this would greatly benefit from this being more structured, because state management for this looks like it will be painful. To provide the additional structure, without reinventing the wheel, the Contextual Links JS, Quick Edit JS, CKEditor toolbar configuration UI JS and Toolbar JS have all been written using Backbone models & views.
@@ -0,0 +1,139 @@
+ data.$el.find('.outside-inblock-configure a').click(function () {
+ if (!isActiveMode()) {
+ $('div.contextual-toolbar-tab.toolbar-tab button').click();
+ }
+ });
This code signals very strongly to me that we indeed want Backbone-based code. Because this looks a lot like the unmaintainable JS code of the past.
b/core/modules/outside_in/js/outside_in.js
@@ -0,0 +1,139 @@
+ var isActiveMode = function () {
+ return $('#toolbar-bar').hasClass('js-outside-in-edit-mode');
+ };And this is the sort of "brittle DOM-based state tracking" that you can avoid by using Backbone.
@drpal(yes not a mis-spelling ;) ) how did most of the JS concured
Yes. I absolutely agree that this work should be encapsulated with Backbone models & views
Let's do it!
| Comment | File | Size | Author |
|---|---|---|---|
| #25 | 2784935-25.patch | 28.29 KB | tedbow |
| #21 | 2784935-21.patch | 28.07 KB | tedbow |
| #21 | interdiff-19-21.txt | 4.85 KB | tedbow |
| #19 | 2784935-19.patch | 27.78 KB | GrandmaGlassesRopeMan |
| #15 | 2784935-15.patch | 27.83 KB | tedbow |
Comments
Comment #2
webchickComment #3
tedbowComment #4
tedbowThis seems important to do sooner rather than later. As it will affect other JS
Comment #5
droplet CreditAttribution: droplet commentedDon't use framework because you just want to use framework :)
http://cgit.drupalcode.org/drupal/tree/core/modules/outside_in/js/outsid...
This is 147 LINE.
If this module has no secrets plan to expand its features;
If you don't use Backbone.Router;
If you don't use Backbone.Model in Backbone way;
If you don't use Backbone.View to do critical frontend rendering;
If you don't use Backbone.Sync to handle data exchange;
All remaining is a plain object + bundle of custom event listeners; (Or a middleware function to handle & dispatch the actions). This is how Backbone works also, doesn't it? Backbone has no reactive & 2-way data binding.
Comment #6
tedbow@droplet re:
No secret plan but #2784443: Move off-canvas functionality from Settings tray module into drupal.dialog.ajax library so that other modules can use it
The idea is the offcanvas tray will be an available dialog type in core like "modal". Then other parts of core and contrib would start to use it. We are already starting to work on the Place Block module to use it: #2784495: Normalize block place and outside-in experiences
I think the offcanvas tray would likely need expand its features for core and contrib uses.
Comment #7
tkoleary CreditAttribution: tkoleary at Acquia commented#2786459: "Offcanvas" tray should be using the existing dialog system Renders this issue moot because (at least as long as we use jquery dialog) jquery will handle state management.
Comment #8
Wim LeersThere's a lot more JS going on than just the dialog-related parts. There's also the reacting to the "Edit" toolbar button etc.
Comment #11
tedbowChanging to new settings_tray.module component. @drpal thanks for script help! :)
Comment #12
fgmProbably no longer applicable if we're going with React/Vue/Polymer/whatever ?
Comment #13
Wim LeersComment #14
Wim LeersIt actually still is IMHO — the current JS is Drupal 6-style JavaScript. Heavily based on storing state in DOM, using jQuery for everything. Brittle because it's easy to get something wrong.
Converting that to Backbone would make the existing code far easier to understand. It'd pave the path for a conversion to React/Vue/Polymer/whatever.
Don't forget that 99% of Settings Tray is still rendered on the server side. React/Vue/Polymer are especially handy for client-side rendering, but Settings Tray doesn't have any of that.
Comment #15
tedbowOk here is my first try at using Backbone.
I started to make a new model and view but then realized that we don't need our own module because if we just want track the "Edit mode" state there is already a model that is doing that,
Drupal.contextualToolbar.model.So this patch follows the pattern of #2784853: Determine when Outside In library should be loaded: piggyback on contextual_toolbar(). We are piggybacking off the Contextual model.
So created a view
Drupal.settings_tray.EditModeViewthat is connected this model. This allows moving a bunch of stuff:const editMode = localStorage.getItem('Drupal.contextualToolbar.isViewing') === 'false';. We don't have to look at local storage at all.setEditModeState(),toggleEditMode(),isInEditMode(), because can just set or check the state of the model.$(toggleEditSelector).once('settingstray').on('click.settingstray', toggleEditMode);We were listening for this event on Contextual's toolbar item. Now our view can justthis.listenTo(this.model, 'change:isViewing', this.render);$(toggleEditSelector).trigger('click').trigger('click.settings_tray');. We were triggering clicking the toolbar "Edit" button when the "Quick Edit" contextual links were clicked. Instead now we can justthis.model.set('isViewing', false);Comment #16
tedbowComment #18
tedbowCancel test b/c against 8.4.x
Comment #19
GrandmaGlassesRopeMan- Applies correctly to 8.5
@tedbow I'll review this soon.
Comment #20
GrandmaGlassesRopeManNever used again.
Excess space.
Multiline comment format.
Needs documentation.
.find should probably be on the same indentation level as .on
Needs a return description for valid JS Doc
Multiline comment format.
Multiline comment format.
Spaces on {}
Comment #21
tedbow@drpal thanks for the review 🙌
1. fixed
2. Not sure where this is. I remove extra lines at the top
3. fixed
4.fixed, also change this function to
setContextualLinksHandler5. fixed
6. fixed.
7. This just moved code but fixed, this actually be put on 1 line with line length of 100.
8. This just moved code but fixed
9. fixed.
Comment #22
droplet CreditAttribution: droplet commentedJust my 2 cents.
To adding Backbone, it needs not move everything into Backbone namespacing. Just make a function call in the right place. Especially now Drupal has a pretty clear vision to move to another framework..
And if our vision really trying to help all frameworks instead one. Soon later, we could split the separated layers into dedicated files, and another non-CORE framework able to reuse the functions without the overhead.
Comment #23
droplet CreditAttribution: droplet commentedRe-thinking if it's over-engineering.
https://twitter.com/NetflixUIE/status/923374215041912833
The last patch only doing one thing with Backbone:
Comment #24
tedbowJust to save someone the click, the quote that @droplet linked to was
From @NetflixUIE
But I don't think this applies here.
Basically to determine whether the user is in "Edit" mode we need to keep a boolean state. We are currently doing this in the module by listing for the Contextual modules edit button to be clicked. then we are also programmaticly click the button to invoke our state.
The thing is the new state we are tracking is actually the same exact state that the Contextual module is already tracking. We don't need our own state.
So we can totally remove the need for Settings Tray to keep track of the state at all and just react to state changes from the contextual model. It already is taking on the complexity of keeping track of the state.
The fact Contextual module and Settings Tray model are keeping track of the same state in 2 different ways means there is always the potential for something to go wrong and for the 2 states to go out of sync which should never happen. If only Contextual was keeping track of the "Edit" mode state then this would not be a potential problem.
Here is reroll, it didn't apply cleanly because of changes to
settings_tray.es6.js#2924351: Fix coding standards issues with existing settings tray JavaScript
#2924365: Wrap JS comments in Settings Tray to 80 character limit
So we would still need to check to see if those need to be fixed in
Comment #25
tedbowForgot to upload the rerolled patch
Comment #33
nod_By now Backbone is on it's way out.