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!

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

tedbow created an issue. See original summary.

webchick’s picture

Issue tags: +JavaScript, +sprint, +Outside-in
tedbow’s picture

Component: javascript » outside_in.module
tedbow’s picture

Priority: Normal » Major
Issue tags: -Outside-in

This seems important to do sooner rather than later. As it will affect other JS

droplet’s picture

Don'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.

var dataModel = {
 // Done.
}
tedbow’s picture

Issue tags: +off-canvas

@droplet re:

If this module has no secrets plan to expand its features;

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.

tkoleary’s picture

Status: Active » Closed (won't fix)

#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.

Wim Leers’s picture

Status: Closed (won't fix) » Active

There's a lot more JS going on than just the dialog-related parts. There's also the reacting to the "Edit" toolbar button etc.

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.

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

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

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

tedbow’s picture

Component: outside_in.module » settings_tray.module

Changing to new settings_tray.module component. @drpal thanks for script help! :)

fgm’s picture

Probably no longer applicable if we're going with React/Vue/Polymer/whatever ?

Wim Leers’s picture

Title: Outside In: Use Backbone for client-side state management » Use Backbone for client-side state management
Wim Leers’s picture

Probably no longer applicable if we're going with React/Vue/Polymer/whatever ?

It 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.

tedbow’s picture

Version: 8.4.x-dev » 8.5.x-dev
FileSize
27.83 KB

Ok 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.EditModeView that is connected this model. This allows moving a bunch of stuff:

  1. const editMode = localStorage.getItem('Drupal.contextualToolbar.isViewing') === 'false';. We don't have to look at local storage at all.
  2. Remove the functions setEditModeState(), toggleEditMode(), isInEditMode(), because can just set or check the state of the model.
  3. $(toggleEditSelector).once('settingstray').on('click.settingstray', toggleEditMode); We were listening for this event on Contextual's toolbar item. Now our view can just this.listenTo(this.model, 'change:isViewing', this.render);
  4. $(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 just this.model.set('isViewing', false);
tedbow’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 15: 2784935-15.patch, failed testing. View results

tedbow’s picture

Status: Needs work » Needs review

Cancel test b/c against 8.4.x

drpal’s picture

- Applies correctly to 8.5

@tedbow I'll review this soon.

drpal’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/settings_tray/js/settings_tray.es6.js
    @@ -7,168 +7,22 @@
       const blockConfigureSelector = '[data-settings-tray-edit]';
    

    Never used again.

  2. +++ b/core/modules/settings_tray/js/settings_tray.es6.js
    @@ -180,61 +34,21 @@
    +
    

    Excess space.

  3. +++ b/core/modules/settings_tray/js/views/EditModeView.es6.js
    @@ -0,0 +1,206 @@
    +      // Since initialize() was triggered in settings_tray.es6.js from drupalContextualLinkAdded
    

    Multiline comment format.

  4. +++ b/core/modules/settings_tray/js/views/EditModeView.es6.js
    @@ -0,0 +1,206 @@
    +    setUpContextualLinks($el) {
    

    Needs documentation.

  5. +++ b/core/modules/settings_tray/js/views/EditModeView.es6.js
    @@ -0,0 +1,206 @@
    +      $el.find(blockConfigureSelector)
    

    .find should probably be on the same indentation level as .on

  6. +++ b/core/modules/settings_tray/js/views/EditModeView.es6.js
    @@ -0,0 +1,206 @@
    +     * @return {Drupal.settings_tray.PageView}
    

    Needs a return description for valid JS Doc

  7. +++ b/core/modules/settings_tray/js/views/EditModeView.es6.js
    @@ -0,0 +1,206 @@
    +      // If there is an element and the renderer is 'off_canvas' then we want
    

    Multiline comment format.

  8. +++ b/core/modules/settings_tray/js/views/EditModeView.es6.js
    @@ -0,0 +1,206 @@
    +        // Loop through all Ajax instances that use the 'off_canvas' renderer to
    

    Multiline comment format.

  9. +++ b/core/modules/settings_tray/js/views/EditModeView.es6.js
    @@ -0,0 +1,206 @@
    +          instance.progress = {type: 'fullscreen'};
    

    Spaces on {}

tedbow’s picture

Status: Needs work » Needs review
FileSize
4.85 KB
28.07 KB

@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 setContextualLinksHandler
5. 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.

droplet’s picture

Just 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.

droplet’s picture

Re-thinking if it's over-engineering.

https://twitter.com/NetflixUIE/status/923374215041912833

The last patch only doing one thing with Backbone:

+++ b/core/modules/settings_tray/js/views/EditModeView.es6.js
@@ -0,0 +1,216 @@
+      this.listenTo(this.model, 'change:isViewing', this.render);
tedbow’s picture

Just to save someone the click, the quote that @droplet linked to was

Removing client-side React.js (but keeping it on the server) resulted in a 50% performance improvement on our landing page

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

core/modules/settings_tray/js/views/EditModeView.es6.js
tedbow’s picture

Forgot to upload the rerolled patch

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.