From 982ae303167fb5e6b4eab5c5582ef88a669e53aa Mon Sep 17 00:00:00 2001 From: Marco Tranchino Date: Mon, 23 Sep 2024 11:32:27 +0100 Subject: [PATCH 1/3] Fix intermittent tests by cleaning up after each --- spec/javascripts/table-of-contents-spec.js | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/spec/javascripts/table-of-contents-spec.js b/spec/javascripts/table-of-contents-spec.js index 0e398557..1d94a753 100644 --- a/spec/javascripts/table-of-contents-spec.js +++ b/spec/javascripts/table-of-contents-spec.js @@ -7,6 +7,7 @@ describe('Table of contents', function () { var $toc var $closeButton var $openButton + var $tocStickyHeader var module beforeAll(function () { @@ -64,13 +65,18 @@ describe('Table of contents', function () { $closeButton = $toc.find('.js-toc-close') $openButton = $html.find('.js-toc-show') + + $tocStickyHeader = $html.find('.toc-show') }) afterEach(function () { // clear up any classes left on - $html.removeClass('.toc-open') + $html.removeClass('toc-open') $html.find('body #toc-heading').remove() $html.find('body .toc').remove() + if ($tocStickyHeader && $tocStickyHeader.length) { + $tocStickyHeader.remove() + } }) describe('when the module is started', function () { From 7d6a9cc2750ddea352f77f2d433906b03d06ea6b Mon Sep 17 00:00:00 2001 From: Marco Tranchino Date: Mon, 23 Sep 2024 15:21:38 +0100 Subject: [PATCH 2/3] Prevent access to this.$sticky unless it is ready This change adds checks to ensure that this.$sticky exists and it is visible before trying to access its methods. Further information at https://github.com/alphagov/tech-docs-gem/pull/368 --- lib/assets/javascripts/_modules/table-of-contents.js | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/lib/assets/javascripts/_modules/table-of-contents.js b/lib/assets/javascripts/_modules/table-of-contents.js index 850d7285..5fe2f780 100644 --- a/lib/assets/javascripts/_modules/table-of-contents.js +++ b/lib/assets/javascripts/_modules/table-of-contents.js @@ -10,7 +10,7 @@ this.isMonitoring = false } StickyOverlapMonitors.prototype.run = function () { - var stickyIsVisible = this.$sticky.is(':visible') + var stickyIsVisible = this.$sticky && this.$sticky.is(':visible') if (stickyIsVisible && !this.isMonitoring) { document.addEventListener('focus', this.onFocus, true) this.isMonitoring = true @@ -26,11 +26,13 @@ if (!applicable) { return } - var stickyEdge = this.$sticky.get(0).getBoundingClientRect().bottom + this.offset - var diff = focused.getBoundingClientRect().top - stickyEdge + if (this.$sticky && this.$sticky.is(':visible') && this.$sticky.get(0)) { + var stickyEdge = this.$sticky.get(0).getBoundingClientRect().bottom + this.offset + var diff = focused.getBoundingClientRect().top - stickyEdge - if (diff < 0) { - $(window).scrollTop($(window).scrollTop() + diff) + if (diff < 0) { + $(window).scrollTop($(window).scrollTop() + diff) + } } } From 96928c9a06df8311d5393854dee6306f7f61b8c9 Mon Sep 17 00:00:00 2001 From: Marco Tranchino Date: Tue, 24 Sep 2024 16:59:22 +0100 Subject: [PATCH 3/3] Update JQuery visibility check on sticky element Following a PR review[1], this commit changes the test on this.$sticky, in order to verify if the object actually contains any elements. Further details in the PR[2]. [1] https://github.com/alphagov/tech-docs-gem/pull/368#issuecomment-2370689455 [2] https://github.com/alphagov/tech-docs-gem/pull/368 --- lib/assets/javascripts/_modules/table-of-contents.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/assets/javascripts/_modules/table-of-contents.js b/lib/assets/javascripts/_modules/table-of-contents.js index 5fe2f780..b24257e7 100644 --- a/lib/assets/javascripts/_modules/table-of-contents.js +++ b/lib/assets/javascripts/_modules/table-of-contents.js @@ -10,7 +10,7 @@ this.isMonitoring = false } StickyOverlapMonitors.prototype.run = function () { - var stickyIsVisible = this.$sticky && this.$sticky.is(':visible') + var stickyIsVisible = this.$sticky.length > 0 && this.$sticky.is(':visible') if (stickyIsVisible && !this.isMonitoring) { document.addEventListener('focus', this.onFocus, true) this.isMonitoring = true