Problem/Motivation

Settings Tray contains minor coding standards issues in it's CSS and JavaScript.

Proposed resolution

- Fix JavaScript coding standards issues.
- Add CSS cross-browser prefixes.
- Cleanup comments.

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

Comments

droplet created an issue. See original summary.

droplet’s picture

Title: Fix comment CSS & JS errors in outside_in module » Fix common CSS & JS errors in outside_in module
Issue summary: View changes
drpal’s picture

Status: Needs review » Needs work

I don't have any opinion on the CSS changes, and just some minor points on the JS fixes.

  1. +++ b/core/modules/outside_in/js/off-canvas.es6.js
    @@ -11,7 +11,7 @@
       // The minimum width to use body displace needs to match the width at which
    

    Use multi-line comment format.

  2. +++ b/core/modules/outside_in/js/outside_in.es6.js
    @@ -184,6 +127,63 @@
    +    // Bind Ajax behaviors to all items showing the class.
    

    Use multi-line comment format.

  3. +++ b/core/modules/outside_in/js/outside_in.es6.js
    @@ -184,6 +127,63 @@
    +    // Bind a listener to all 'Quick edit' links for blocks
    

    Use multi-line comment format.

  4. +++ b/core/modules/outside_in/js/outside_in.es6.js
    @@ -184,6 +127,63 @@
    +        // Always disable QuickEdit regardless of whether "EditMode" was just enabled.
    

    80 char.

drpal’s picture

Status: Needs work » Needs review
FileSize
2.72 KB
24.26 KB

Fixed 1,2,3,4-#3

Wim Leers’s picture

So, #4 looks great to me.

@drpal already reviewed the JS changes that @droplet is proposing and effectively RTBC'd them.

Hopefully @DyanneNova's knowledge about Settings Tray's CSS means she can then RTBC this patch :)

droplet’s picture

Welcome All CSS developers to review it. It's generic problems :)

DyanneNova’s picture

I don't think we should decide the font stack in this issue. I can see why the current stack was chosen, but that's probably best left up to discussion in the other issue. I agree that it would be nice to standardize where we're going for the same effect.

We shouldn't need a webkit prefix for linear gradient anymore, so I removed those lines.

Everything else looks good to me.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Let's do this then!

Cottser’s picture

Status: Reviewed & tested by the community » Needs work

Found some minor docs things. I hope to be able to do another review in the next day or two but at a glance, the changes are looking good.

There are an awful lot of IDs in this CSS, but maybe that can be discussed on a separate issue. I'm also not sure of the history behind that, and it'd be a riskier change because the specificity would be reduced if we changed to a data attribute or class.

  1. +++ b/core/modules/outside_in/js/outside_in.es6.js
    @@ -184,6 +130,69 @@
    +   * @return {boolean}
    +   *  State of the outside-in edit mode.
    +   */
    +  function isInEditMode() {
    

    This @return description ("State of the…") should be indented by one more space.

  2.   /**
       *  Helper to switch edit mode state.
       *
       * @param {boolean} editMode
       *  True enable edit mode, false disable edit mode.
       */
      function setEditModeState(editMode) {
    • A. The summary line here is indented one space too many ("Helper to switch…")
    • B. The @param description should be indented one more space
  3. off-canvas.theme.css is missing a @file docblock
  4. off-canvas.base.css @file docblock starts with /*, should start with /**
  5. off-canvas.button.css kinda has a @file docblock but not an actual @file
drpal’s picture

Status: Needs work » Needs review
FileSize
1.91 KB
19.66 KB

- fixes for #9-1, 2.a, 2.b, 3, 4, 5

drpal’s picture

FileSize
1.91 KB

- actual interdiff from #10.

Cottser’s picture

Status: Needs review » Reviewed & tested by the community

That was quick, thanks @drpal! The changes all look great. Back to RTBC for now.

droplet’s picture

There are an awful lot of IDs in this CSS, but maybe that can be discussed on a separate issue. I'm also not sure of the history behind that

There's no history if we do the changes now :p The code was introduced from below issue:
#2826722: Add a 'fence' around settings tray with aggressive CSS reset.

I agree to do it in a separate issue

drpal’s picture

@Cottser 🙏

lauriii’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs issue summary update

I was trying to review this issue. The changes itself look good, but I couldn't tell if the patch fixes the issue in it's the whole scope since I don't fully understand the scope. No need for a long issue summary but I would appreciate if someone who knows more about this issue could spend few minutes writing down the scope to the issue summary.

droplet’s picture

(my) the scope is as simple as 3 points I listed. Just do a quick fix. -- Let's call it a QC checking :P

drpal’s picture

Title: Fix common CSS & JS errors in outside_in module » Fixes for settings tray CSS and JavaScript minor issues.
Issue summary: View changes
tedbow’s picture

Status: Needs review » Reviewed & tested by the community

@drpal @droplet thanks for comments and updates. Back to RTBC

Cottser’s picture

Title: Fixes for settings tray CSS and JavaScript minor issues. » Fixes for settings tray CSS and JavaScript minor issues
Status: Reviewed & tested by the community » Needs work
Issue tags: -Needs issue summary update

Thanks, everyone :)

+++ b/core/modules/outside_in/js/off-canvas.es6.js
@@ -10,8 +10,10 @@
-  // The minimum width to use body displace needs to match the width at which
-  // the tray will be %100 width. @see outside_in.module.css
+  /**
+   * The minimum width to use body displace needs to match the width at which
+   * the off-canvas dialog will be 100% width. @see outside_in.module.css
+   */

@@ -69,13 +71,12 @@
-    let modalHeight;
+    const modalHeight = $widget.height();
...
-    modalHeight = $widget.height();
-    $offsets.each(function () {
-      offset += $(this).outerHeight();
+    $offsets.each((index, element) => {
+      offset += $(element).outerHeight();

@@ -116,7 +117,7 @@
-        'dialog:aftercreate': function (event, dialog, $element, settings) {
+        'dialog:aftercreate': (event, dialog, $element, settings) => {

@@ -134,7 +135,7 @@
-        'dialog:beforecreate': function (event, dialog, $element, settings) {
+        'dialog:beforecreate': (event, dialog, $element, settings) => {

@@ -149,7 +150,7 @@
-        'dialog:beforeclose': function (event, dialog, $element) {
+        'dialog:beforeclose': (event, dialog, $element) => {

Let's leave these changes for #2830882: Introduce Drupal.offCanvas, mirrored after Drupal.dialog, to avoid Settings Tray using Drupal.Dialog in unintended ways (the code changes look to be taken care of there) or #2784443: Move off-canvas functionality from Settings tray module into drupal.dialog.ajax library so that other modules can use it (a similar docs change is in the patch there).

Other than that this looks good, also this needs a reroll because of #2901205: Settings tray leaves dashed outline on followup pages, that's my fault.

tedbow’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
20.77 KB

@Cottser thanks for your on Settings Tray issues

Rerolled

Cottser’s picture

Status: Reviewed & tested by the community » Needs work

Thanks @tedbow :)

The conflicting changes I mentioned are still in the patch so this needs a reroll because I just committed #2830882: Introduce Drupal.offCanvas, mirrored after Drupal.dialog, to avoid Settings Tray using Drupal.Dialog in unintended ways. Might have needed a reroll either way though.

tedbow’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
17.6 KB

@Cottser no problem! excited to get all these Settings Tray commits in!

This is reroll of #20. It has all the changes except to off-canvas.js and off-canvas.es6.js. I went through each of the changes and they were already there because of #2830882: Introduce Drupal.offCanvas, mirrored after Drupal.dialog, to avoid Settings Tray using Drupal.Dialog in unintended ways which cleaned up all the js in those files.

  • Cottser committed 8972a66 on 8.5.x
    Issue #2899724 by drpal, tedbow, DyanneNova, droplet, Cottser, Wim Leers...
Cottser’s picture

Status: Reviewed & tested by the community » Fixed

I verified the changes from #20 to #22 myself, so +1 to the RTBC.

Committed and pushed 8972a66f2e to 8.5.x and 9ba27699bd to 8.4.x. Thanks!