From 7ce70dd5a046048ef13eb5f93ece8f149bfbaea3 Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Thu, 14 Sep 2017 20:25:48 +0100 Subject: [PATCH 1/6] Dynamically create offset for sticky bar --- app/assets/javascripts/lib/utils/sticky.js | 30 ++++++++++++++++++---- app/assets/stylesheets/pages/diff.scss | 9 ++----- 2 files changed, 27 insertions(+), 12 deletions(-) diff --git a/app/assets/javascripts/lib/utils/sticky.js b/app/assets/javascripts/lib/utils/sticky.js index 283c0ec0410..f8f0a43f1cc 100644 --- a/app/assets/javascripts/lib/utils/sticky.js +++ b/app/assets/javascripts/lib/utils/sticky.js @@ -1,14 +1,34 @@ -export const isSticky = (el, scrollY, stickyTop) => { +export const createPlaceholder = () => { + const placeholder = document.createElement('div'); + placeholder.classList.add('sticky-placeholder'); + + return placeholder; +}; + +export const isSticky = (el, scrollY, stickyTop, insertPlaceholder) => { const top = Math.floor(el.offsetTop - scrollY); - if (top <= stickyTop) { + if (top <= stickyTop && !el.classList.contains('is-stuck')) { + const placeholder = insertPlaceholder ? createPlaceholder(el) : null; + const heightBefore = el.offsetHeight; + el.classList.add('is-stuck'); - } else { + + if (insertPlaceholder) { + el.parentNode.insertBefore(placeholder, el.nextElementSibling); + + placeholder.style.height = `${heightBefore - el.offsetHeight}px`; + } + } else if (top > stickyTop && el.classList.contains('is-stuck')) { el.classList.remove('is-stuck'); + + if (insertPlaceholder && el.nextElementSibling.classList.contains('sticky-placeholder')) { + el.nextElementSibling.remove(); + } } }; -export default (el) => { +export default (el, insertPlaceholder = true) => { if (!el) return; const computedStyle = window.getComputedStyle(el); @@ -17,7 +37,7 @@ export default (el) => { const stickyTop = parseInt(computedStyle.top, 10); - document.addEventListener('scroll', () => isSticky(el, window.scrollY, stickyTop), { + document.addEventListener('scroll', () => isSticky(el, window.scrollY, stickyTop, insertPlaceholder), { passive: true, }); }; diff --git a/app/assets/stylesheets/pages/diff.scss b/app/assets/stylesheets/pages/diff.scss index 951580ea1fe..5dbf44073b5 100644 --- a/app/assets/stylesheets/pages/diff.scss +++ b/app/assets/stylesheets/pages/diff.scss @@ -451,7 +451,7 @@ } .files { - margin-top: -1px; + margin-top: 1px; .diff-file:last-child { margin-bottom: 0; @@ -586,11 +586,6 @@ top: 76px; } - + .files, - + .alert { - margin-top: 1px; - } - &:not(.is-stuck) .diff-stats-additions-deletions-collapsed { display: none; } @@ -608,7 +603,7 @@ + .files, + .alert { - margin-top: 32px; + // margin-top: 32px; } } } From 734bb736258293e213fb0580f1f0b15618a78bc2 Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Fri, 15 Sep 2017 09:01:58 +0100 Subject: [PATCH 2/6] fixed and added specs for removing placeholder element --- spec/javascripts/lib/utils/sticky_spec.js | 40 +++++++++++++++++++++-- 1 file changed, 37 insertions(+), 3 deletions(-) diff --git a/spec/javascripts/lib/utils/sticky_spec.js b/spec/javascripts/lib/utils/sticky_spec.js index c3ee3ef9825..5ffc41589e5 100644 --- a/spec/javascripts/lib/utils/sticky_spec.js +++ b/spec/javascripts/lib/utils/sticky_spec.js @@ -4,12 +4,25 @@ describe('sticky', () => { const el = { offsetTop: 0, classList: {}, + parentNode: {}, + nextElementSibling: {}, }; + let isStuck = false; beforeEach(() => { el.offsetTop = 0; - el.classList.add = jasmine.createSpy('spy'); - el.classList.remove = jasmine.createSpy('spy'); + el.classList.add = jasmine.createSpy('classListAdd'); + el.classList.remove = jasmine.createSpy('classListRemove'); + el.classList.contains = jasmine.createSpy('classListContains').and.callFake(() => isStuck); + el.parentNode.insertBefore = jasmine.createSpy('insertBefore'); + el.nextElementSibling.remove = jasmine.createSpy('nextElementSibling'); + el.nextElementSibling.classList = { + contains: jasmine.createSpy('classListContains').and.returnValue(true), + }; + }); + + afterEach(() => { + isStuck = false; }); describe('classList.remove', () => { @@ -21,7 +34,9 @@ describe('sticky', () => { ).not.toHaveBeenCalled(); }); - it('calls classList.remove when not stuck', () => { + it('calls classList.remove when no longer stuck', () => { + isStuck = true; + el.offsetTop = 10; isSticky(el, 0, 0); @@ -29,6 +44,17 @@ describe('sticky', () => { el.classList.remove, ).toHaveBeenCalledWith('is-stuck'); }); + + it('removes placeholder when no longer stuck', () => { + isStuck = true; + + el.offsetTop = 10; + isSticky(el, 0, 0, true); + + expect( + el.nextElementSibling.remove, + ).toHaveBeenCalled(); + }); }); describe('classList.add', () => { @@ -48,5 +74,13 @@ describe('sticky', () => { el.classList.add, ).not.toHaveBeenCalled(); }); + + it('inserts placeholder element when stuck', () => { + isSticky(el, 0, 0, true); + + expect( + el.parentNode.insertBefore, + ).toHaveBeenCalled(); + }); }); }); From da6ff6519818fafe38433599b663a4aeff1007c1 Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Thu, 21 Sep 2017 08:04:25 +0100 Subject: [PATCH 3/6] spec fixes --- app/assets/stylesheets/test.scss | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/app/assets/stylesheets/test.scss b/app/assets/stylesheets/test.scss index 7d9f3da79c5..06733b7f1a9 100644 --- a/app/assets/stylesheets/test.scss +++ b/app/assets/stylesheets/test.scss @@ -15,3 +15,9 @@ -ms-animation: none !important; animation: none !important; } + +// Disable sticky changes bar for tests +.diff-files-changed { + position: relative !important; + top: 0 !important; +} From 65415e3208c29f52e3da6ccbdcdabcdf00bdb67b Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Mon, 25 Sep 2017 10:57:23 +0100 Subject: [PATCH 4/6] removed commented out CSS --- app/assets/javascripts/lib/utils/sticky.js | 2 +- app/assets/stylesheets/pages/diff.scss | 5 ----- 2 files changed, 1 insertion(+), 6 deletions(-) diff --git a/app/assets/javascripts/lib/utils/sticky.js b/app/assets/javascripts/lib/utils/sticky.js index f8f0a43f1cc..2803939c7cd 100644 --- a/app/assets/javascripts/lib/utils/sticky.js +++ b/app/assets/javascripts/lib/utils/sticky.js @@ -9,7 +9,7 @@ export const isSticky = (el, scrollY, stickyTop, insertPlaceholder) => { const top = Math.floor(el.offsetTop - scrollY); if (top <= stickyTop && !el.classList.contains('is-stuck')) { - const placeholder = insertPlaceholder ? createPlaceholder(el) : null; + const placeholder = insertPlaceholder ? createPlaceholder() : null; const heightBefore = el.offsetHeight; el.classList.add('is-stuck'); diff --git a/app/assets/stylesheets/pages/diff.scss b/app/assets/stylesheets/pages/diff.scss index 5dbf44073b5..ed9d5e98467 100644 --- a/app/assets/stylesheets/pages/diff.scss +++ b/app/assets/stylesheets/pages/diff.scss @@ -600,11 +600,6 @@ .inline-parallel-buttons { display: none; } - - + .files, - + .alert { - // margin-top: 32px; - } } } } From f389f9081f92b68d8d8369d29e8f05a65f47e9dc Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Mon, 25 Sep 2017 11:40:40 +0100 Subject: [PATCH 5/6] refactor tests to actually test browser behaviour --- app/assets/javascripts/lib/utils/sticky.js | 2 +- spec/javascripts/lib/utils/sticky_spec.js | 99 ++++++++++------------ 2 files changed, 45 insertions(+), 56 deletions(-) diff --git a/app/assets/javascripts/lib/utils/sticky.js b/app/assets/javascripts/lib/utils/sticky.js index 2803939c7cd..64db42701ce 100644 --- a/app/assets/javascripts/lib/utils/sticky.js +++ b/app/assets/javascripts/lib/utils/sticky.js @@ -22,7 +22,7 @@ export const isSticky = (el, scrollY, stickyTop, insertPlaceholder) => { } else if (top > stickyTop && el.classList.contains('is-stuck')) { el.classList.remove('is-stuck'); - if (insertPlaceholder && el.nextElementSibling.classList.contains('sticky-placeholder')) { + if (insertPlaceholder && el.nextElementSibling && el.nextElementSibling.classList.contains('sticky-placeholder')) { el.nextElementSibling.remove(); } } diff --git a/spec/javascripts/lib/utils/sticky_spec.js b/spec/javascripts/lib/utils/sticky_spec.js index 5ffc41589e5..8ddf86c4ee4 100644 --- a/spec/javascripts/lib/utils/sticky_spec.js +++ b/spec/javascripts/lib/utils/sticky_spec.js @@ -1,86 +1,75 @@ import { isSticky } from '~/lib/utils/sticky'; describe('sticky', () => { - const el = { - offsetTop: 0, - classList: {}, - parentNode: {}, - nextElementSibling: {}, - }; - let isStuck = false; + let el; beforeEach(() => { - el.offsetTop = 0; - el.classList.add = jasmine.createSpy('classListAdd'); - el.classList.remove = jasmine.createSpy('classListRemove'); - el.classList.contains = jasmine.createSpy('classListContains').and.callFake(() => isStuck); - el.parentNode.insertBefore = jasmine.createSpy('insertBefore'); - el.nextElementSibling.remove = jasmine.createSpy('nextElementSibling'); - el.nextElementSibling.classList = { - contains: jasmine.createSpy('classListContains').and.returnValue(true), - }; + document.body.innerHTML = ` +
+
+
+ `; + + el = document.getElementById('js-sticky'); }); - afterEach(() => { - isStuck = false; - }); - - describe('classList.remove', () => { - it('does not call classList.remove when stuck', () => { - isSticky(el, 0, 0); + describe('when stuck', () => { + it('does not remove is-stuck class', () => { + isSticky(el, 0, el.offsetTop); + isSticky(el, 0, el.offsetTop); expect( - el.classList.remove, - ).not.toHaveBeenCalled(); + el.classList.contains('is-stuck'), + ).toBeTruthy(); }); - it('calls classList.remove when no longer stuck', () => { - isStuck = true; + it('adds is-stuck class', () => { + isSticky(el, 0, el.offsetTop); - el.offsetTop = 10; + expect( + el.classList.contains('is-stuck'), + ).toBeTruthy(); + }); + + it('inserts placeholder element', () => { + isSticky(el, 0, el.offsetTop, true); + + expect( + document.querySelector('.sticky-placeholder'), + ).not.toBeNull(); + }); + }); + + describe('when not stuck', () => { + it('removes is-stuck class', () => { + spyOn(el.classList, 'remove').and.callThrough(); + + isSticky(el, 0, el.offsetTop); isSticky(el, 0, 0); expect( el.classList.remove, ).toHaveBeenCalledWith('is-stuck'); - }); - - it('removes placeholder when no longer stuck', () => { - isStuck = true; - - el.offsetTop = 10; - isSticky(el, 0, 0, true); - expect( - el.nextElementSibling.remove, - ).toHaveBeenCalled(); + el.classList.contains('is-stuck'), + ).toBeFalsy(); }); - }); - describe('classList.add', () => { - it('calls classList.add when stuck', () => { + it('does not add is-stuck class', () => { isSticky(el, 0, 0); expect( - el.classList.add, - ).toHaveBeenCalledWith('is-stuck'); + el.classList.contains('is-stuck'), + ).toBeFalsy(); }); - it('does not call classList.add when not stuck', () => { - el.offsetTop = 10; - isSticky(el, 0, 0); - - expect( - el.classList.add, - ).not.toHaveBeenCalled(); - }); - - it('inserts placeholder element when stuck', () => { + it('removes placeholder', () => { + isSticky(el, 0, el.offsetTop, true); isSticky(el, 0, 0, true); expect( - el.parentNode.insertBefore, - ).toHaveBeenCalled(); + document.querySelector('.sticky-placeholder'), + ).toBeNull(); }); }); }); From 1544bc32283d791a5094a6eedc0df344462e2c31 Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Mon, 25 Sep 2017 11:55:33 +0100 Subject: [PATCH 6/6] remove the sticky el from the test DOM after each test --- spec/javascripts/lib/utils/sticky_spec.js | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/spec/javascripts/lib/utils/sticky_spec.js b/spec/javascripts/lib/utils/sticky_spec.js index 8ddf86c4ee4..b87c836654d 100644 --- a/spec/javascripts/lib/utils/sticky_spec.js +++ b/spec/javascripts/lib/utils/sticky_spec.js @@ -4,15 +4,19 @@ describe('sticky', () => { let el; beforeEach(() => { - document.body.innerHTML = ` + document.body.innerHTML += `
-
+
`; el = document.getElementById('js-sticky'); }); + afterEach(() => { + el.parentNode.remove(); + }); + describe('when stuck', () => { it('does not remove is-stuck class', () => { isSticky(el, 0, el.offsetTop);