From 177f30248e342c20f4e1848b34d8ee0c510b46b6 Mon Sep 17 00:00:00 2001 From: Tom Byers Date: Wed, 30 Jan 2019 11:35:31 +0000 Subject: [PATCH 1/4] Fix bug with DOM sync'ing If a sticky element was already in the store, the code for adding it would return early. This meant dimensions and positions for it were not being recalculated. --- .../stick-to-window-when-scrolling.js | 24 ++++++++++++------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/app/assets/javascripts/stick-to-window-when-scrolling.js b/app/assets/javascripts/stick-to-window-when-scrolling.js index 63914fce0..e15bddfa4 100644 --- a/app/assets/javascripts/stick-to-window-when-scrolling.js +++ b/app/assets/javascripts/stick-to-window-when-scrolling.js @@ -413,30 +413,38 @@ Sticky.prototype.allElementsLoaded = function (totalEls) { return this._els.length === totalEls; }; - Sticky.prototype.hasEl = function (node) { - return !!$.grep(this._els, function (el) { return el.$fixedEl.is(node); }).length; + Sticky.prototype.getElForNode = function (node) { + var matches = $.grep(this._els, function (el) { return el.$fixedEl.is(node); }); + + return !!matches.length ? matches[0] : false; }; Sticky.prototype.add = function (el, setPositions, cb) { var self = this; var $el = $(el); var onDimensionsSet; - var elObj; - - // guard against adding elements already stored - if (this.hasEl(el)) { return; } + var elObj = this.getElForNode(el); + var exists = !!elObj; onDimensionsSet = function () { elObj.hasLoaded(true); - self._els.push(elObj); + + // guard against adding elements already stored + if (!exists) { + self._els.push(elObj); + } + if (setPositions) { self.setElementPositions(); } + if (cb !== undefined) { cb(); } }; - elObj = new StickyElement($el, self); + if (!exists) { + elObj = new StickyElement($el, self); + } self.setElementDimensions(elObj, onDimensionsSet); }; From f4d9c379403d102350cea4e8ad7300b86bd94473 Mon Sep 17 00:00:00 2001 From: Tom Byers Date: Wed, 30 Jan 2019 11:45:24 +0000 Subject: [PATCH 2/4] Move setting of sticky mode out of recalculate The `recalculate` method currently does three things: 1. sync's the internal store with the DOM 2. updates the saved positional and dimensional data from the new DOM 3. allows the mode to be set The problem with using it as the way to set the mode is that, every call to it is effectively setting the mode but this isn't always the intention. This splits off setting the mode so other modules have to explicity set it and those that don't intend to can just call `recalculate` to notify of DOM changes. --- .../javascripts/stick-to-window-when-scrolling.js | 11 ++++++----- app/assets/javascripts/templateFolderForm.js | 7 +++++-- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/app/assets/javascripts/stick-to-window-when-scrolling.js b/app/assets/javascripts/stick-to-window-when-scrolling.js index e15bddfa4..bcc413084 100644 --- a/app/assets/javascripts/stick-to-window-when-scrolling.js +++ b/app/assets/javascripts/stick-to-window-when-scrolling.js @@ -272,6 +272,9 @@ this.CSS_SELECTOR = selector; this.STOP_PADDING = 10; }; + Sticky.prototype.setMode = function (mode) { + _mode = mode; + }; Sticky.prototype.getWindowDimensions = function () { return { height: $(global).height(), @@ -360,7 +363,7 @@ } }; // Recalculate stored dimensions for all sticky elements - Sticky.prototype.recalculate = function (opts) { + Sticky.prototype.recalculate = function () { var self = this; var onSyncComplete = function () { self.setEvents(); @@ -371,8 +374,6 @@ self.setElementPositions(); }; - if ((opts !== undefined) && ('mode' in opts)) { _mode = opts.mode; } - this.syncWithDOM(onSyncComplete); }; Sticky.prototype.setElWidth = function (el) { @@ -493,8 +494,8 @@ }); } }; - Sticky.prototype.init = function (opts) { - this.recalculate(opts); + Sticky.prototype.init = function () { + this.recalculate(); }; Sticky.prototype.setEvents = function () { var self = this; diff --git a/app/assets/javascripts/templateFolderForm.js b/app/assets/javascripts/templateFolderForm.js index dcd91a5a1..b7b0cf43b 100644 --- a/app/assets/javascripts/templateFolderForm.js +++ b/app/assets/javascripts/templateFolderForm.js @@ -144,9 +144,12 @@ state => (state.key === this.currentState ? this.$stickyBottom.append(state.$el) : state.$el.detach()) ); - // make sticky JS recalculate its cache of the element's position // use dialog mode for states which contain more than one form control - GOVUK.stickAtBottomWhenScrolling.recalculate({ 'mode': 'dialog' }); + if (['move-to-existing-folder', 'add-new-template'].includes(this.currentState)) { + GOVUK.stickAtBottomWhenScrolling.setMode('dialog'); + } + // make sticky JS recalculate its cache of the element's position + GOVUK.stickAtBottomWhenScrolling.recalculate(); }; this.nothingSelectedButtons = $(` From 707c426b9aa5b7730ac49dff21763b27507d08bd Mon Sep 17 00:00:00 2001 From: Tom Byers Date: Wed, 30 Jan 2019 14:10:59 +0000 Subject: [PATCH 3/4] Remove padding between stickys when stacked --- .../stick-to-window-when-scrolling.js | 16 ++++++++++++++-- .../components/stick-at-top-when-scrolling.scss | 4 ++++ 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/app/assets/javascripts/stick-to-window-when-scrolling.js b/app/assets/javascripts/stick-to-window-when-scrolling.js index bcc413084..e5b37e86e 100644 --- a/app/assets/javascripts/stick-to-window-when-scrolling.js +++ b/app/assets/javascripts/stick-to-window-when-scrolling.js @@ -141,6 +141,15 @@ // were wrapped by a dialog component var dialog = { _hasResized: false, + // we add padding of 20px around each sticky to isolate it from the rest of the page + // it shouldn't apply between stickys when stacked + _getPaddingBetweenEls: function (els) { + var spaceBetween = 40; + + if (els.length < 2) { return 0; } + + return (els.length - 1) * spaceBetween; + }, _getTotalHeight: function (els) { var reducer = function (accumulator, currentValue) { return accumulator + currentValue; @@ -152,6 +161,7 @@ }, getOffsetFromEdge: function (el, sticky) { var els = this._elsThatCanBeStuck(sticky._els).slice(); + var elsBetween; var elIdx; // els must be arranged furtherest from window edge is stuck to first @@ -165,10 +175,12 @@ // if next to window edge the dialog is stuck to, no offset if (elIdx === (els.length - 1)) { return 0; } + // make els all those from this one to the window edge + els = els.slice(elIdx); // get all els between this one and the window edge - els = els.slice(elIdx + 1); + elsBetween = els.slice(1); - return this._getTotalHeight(els); + return this._getTotalHeight(elsBetween) - this._getPaddingBetweenEls(els); }, getOffsetFromEnd: function (el, sticky) { var els = this._elsThatCanBeStuck(sticky._els).slice(); diff --git a/app/assets/stylesheets/components/stick-at-top-when-scrolling.scss b/app/assets/stylesheets/components/stick-at-top-when-scrolling.scss index 9c5cea1f1..1c0b85cf7 100644 --- a/app/assets/stylesheets/components/stick-at-top-when-scrolling.scss +++ b/app/assets/stylesheets/components/stick-at-top-when-scrolling.scss @@ -46,6 +46,10 @@ padding: $vertical-padding 0 $vertical-padding $gutter-half; margin-top: -$vertical-padding; + & + .js-stick-at-bottom-when-scrolling { + margin-top: ($vertical-padding * 2) * -1; + } + .page-footer { margin-bottom: 0; min-height: 50px; From c89ad9635bcc13dcbabae3b84ae9cf5df299c220 Mon Sep 17 00:00:00 2001 From: Tom Byers Date: Wed, 30 Jan 2019 19:12:14 +0000 Subject: [PATCH 4/4] Fix switch between modes and scrolling on adjust The templateFolderForm JS was setting the mode to 'dialog' but not back when the state was changed back to a normal sticky element. This caused adjustments and scrolling when no adjustment needed to be accommodated. There were also problems with adjustForResize. It was manipulating the same flag that resizing the window did and returned a value never used. --- .../stick-to-window-when-scrolling.js | 24 ++++++++++--------- app/assets/javascripts/templateFolderForm.js | 5 +++- 2 files changed, 17 insertions(+), 12 deletions(-) diff --git a/app/assets/javascripts/stick-to-window-when-scrolling.js b/app/assets/javascripts/stick-to-window-when-scrolling.js index e5b37e86e..5b1568b52 100644 --- a/app/assets/javascripts/stick-to-window-when-scrolling.js +++ b/app/assets/javascripts/stick-to-window-when-scrolling.js @@ -140,7 +140,7 @@ // Object collecting together methods for treating sticky elements as if they // were wrapped by a dialog component var dialog = { - _hasResized: false, + hasResized: false, // we add padding of 20px around each sticky to isolate it from the rest of the page // it shouldn't apply between stickys when stacked _getPaddingBetweenEls: function (els) { @@ -230,10 +230,8 @@ sticky.reset(currentEl); currentEl.canBeStuck(false); - if (!sticky._hasResized) { sticky._hasResized = true; } + if (!self.hasResized) { self.hasResized = true; } } - - return this._getTotalHeight(els); }, getElementAtStickyEdge: function (sticky) { var els = this._elsThatCanBeStuck(sticky._els); @@ -263,7 +261,7 @@ $(window).scrollTop(this.getInPageEdgePosition(sticky) - windowHeight); } - sticky._hasResized = false; + self.hasResized = false; }, releaseEl: function (el, sticky) { el.$fixedEl.css(sticky.edge, ''); @@ -275,7 +273,7 @@ var Sticky = function (selector) { this._hasScrolled = false; this._scrollTimeout = false; - this._hasResized = false; + this._windowHasResized = false; this._resizeTimeout = false; this._elsLoaded = false; this._initialPositionsSet = false; @@ -381,7 +379,9 @@ self.setEvents(); if (_mode === 'dialog') { dialog.fitToHeight(self); - dialog.adjustForResize(self); + if (dialog.hasResized) { + dialog.adjustForResize(self); + } } self.setElementPositions(); }; @@ -532,7 +532,7 @@ this._hasScrolled = true; }; Sticky.prototype.onResize = function () { - this._hasResized = true; + this._windowHasResized = true; }; Sticky.prototype.checkScroll = function () { var self = this; @@ -546,8 +546,8 @@ var self = this, windowWidth = self.getWindowDimensions().width; - if (self._hasResized === true) { - self._hasResized = false; + if (self._windowHasResized === true) { + self._windowHasResized = false; $.each(self._els, function (i, el) { if (!self.viewportIsWideEnough(windowWidth)) { @@ -560,7 +560,9 @@ if (self.viewportIsWideEnough(windowWidth)) { if (_mode === 'dialog') { dialog.fitToHeight(self); - dialog.adjustForResize(self); + if (dialog.hasResized) { + dialog.adjustForResize(self); + } } self.setElementPositions(); } diff --git a/app/assets/javascripts/templateFolderForm.js b/app/assets/javascripts/templateFolderForm.js index b7b0cf43b..9233c88c1 100644 --- a/app/assets/javascripts/templateFolderForm.js +++ b/app/assets/javascripts/templateFolderForm.js @@ -139,6 +139,8 @@ }; this.render = function() { + var mode = 'default'; + // detach everything, unless they are the currentState this.states.forEach( state => (state.key === this.currentState ? this.$stickyBottom.append(state.$el) : state.$el.detach()) @@ -146,8 +148,9 @@ // use dialog mode for states which contain more than one form control if (['move-to-existing-folder', 'add-new-template'].includes(this.currentState)) { - GOVUK.stickAtBottomWhenScrolling.setMode('dialog'); + mode = 'dialog'; } + GOVUK.stickAtBottomWhenScrolling.setMode(mode); // make sticky JS recalculate its cache of the element's position GOVUK.stickAtBottomWhenScrolling.recalculate(); };