Problem/Motivation

In jQuery 3.0 the behavior of .outerHeight() has changed to return NaN when an element is not found.

Proposed resolution

If an element is not returned from the selector, use a 0 value.

CommentFileSizeAuthor
toolbar.patch1.75 KBdroplet
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

droplet created an issue. See original summary.

GrandmaGlassesRopeMan’s picture

GrandmaGlassesRopeMan
he/him
Primary language English
Location 🌎
CreditAttribution: GrandmaGlassesRopeMan at Acquia commented

@droplet 👍 This is the same conclusion I just realized.

When closing the toolbar, $('.is-active.toolbar-tray-horizontal') returns nothing, thus .outerHeight() returns NaN breaking the height set.

GrandmaGlassesRopeMan’s picture

GrandmaGlassesRopeMan
he/him
Primary language English
Location 🌎
CreditAttribution: GrandmaGlassesRopeMan at Acquia commented
Issue summary: View changes
GrandmaGlassesRopeMan’s picture

GrandmaGlassesRopeMan
he/him
Primary language English
Location 🌎
CreditAttribution: GrandmaGlassesRopeMan at Acquia commented
Title: Improve Toolbar Height calculation » Improve Toolbar Height calculation to accommodate jQuery 3.0 changes.
GrandmaGlassesRopeMan’s picture

GrandmaGlassesRopeMan
he/him
Primary language English
Location 🌎
CreditAttribution: GrandmaGlassesRopeMan at Acquia commented
Status: Needs review » Reviewed & tested by the community

🚀

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

lgtm!

martin107’s picture

martin107 CreditAttribution: martin107 as a volunteer commented

looking at the bottom set of changes..

-      this.model.set('height', $('#toolbar-bar').find('.toolbar-tab').outerHeight() + $('.is-active.toolbar-tray-horizontal').outerHeight());
+      var toolbarTabOuterHeight = $('#toolbar-bar').find('.toolbar-tab').outerHeight() || 0;
+      var toolbarTrayHorizontalOuterHeight = $('.is-active.toolbar-tray-horizontal').outerHeight() || 0;
+      this.model.set('height', toolbarTabOuterHeight + toolbarTrayHorizontalOuterHeight);

toolbarTabOuterHeight and toolbarTrayHorizontalOuterHeight are set, not modified and then read.

so like the top set of changes should be defined as const not var.

GrandmaGlassesRopeMan’s picture

GrandmaGlassesRopeMan
he/him
Primary language English
Location 🌎
CreditAttribution: GrandmaGlassesRopeMan at Acquia commented

@martin107

The bottom set of changes, from ToolbarVisualView.js, contain the transpiled JavaScript.

martin107’s picture

martin107 CreditAttribution: martin107 as a volunteer commented

Ah crap, you are right my mistake... sorry for the noise.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick at Acquia commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.4.x. Thanks!

  • webchick committed 478ddd3 on 8.4.x 
    Issue #2894283 by droplet, drpal: Improve Toolbar Height calculation to...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.