Merge branch 'changes-bar-dynamic-placeholder' into 'master'

Dynamically create offset for sticky bar

See merge request gitlab-org/gitlab-ce!14280

4 files changed, 83 insertions, 40 deletions

diff --git a/app/assets/javascripts/lib/utils/sticky.js b/app/assets/javascripts/lib/utils/sticky.js
index 283c0ec0410..64db42701ce 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() : 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 && 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..ed9d5e98467 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;
     }
@@ -605,11 +600,6 @@
       .inline-parallel-buttons {
         display: none;
       }
-
-      + .files,
-      + .alert {
-        margin-top: 32px;
-      }
     }
   }
 }
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;
+}
diff --git a/spec/javascripts/lib/utils/sticky_spec.js b/spec/javascripts/lib/utils/sticky_spec.js
index c3ee3ef9825..b87c836654d 100644
--- a/spec/javascripts/lib/utils/sticky_spec.js
+++ b/spec/javascripts/lib/utils/sticky_spec.js
@@ -1,52 +1,79 @@
 import { isSticky } from '~/lib/utils/sticky';
 
 describe('sticky', () => {
-  const el = {
-    offsetTop: 0,
-    classList: {},
-  };
+  let el;
 
   beforeEach(() => {
-    el.offsetTop = 0;
-    el.classList.add = jasmine.createSpy('spy');
-    el.classList.remove = jasmine.createSpy('spy');
+    document.body.innerHTML += `
+      <div class="parent">
+        <div id="js-sticky"></div>
+      </div>
+    `;
+
+    el = document.getElementById('js-sticky');
   });
 
-  describe('classList.remove', () => {
-    it('does not call classList.remove when stuck', () => {
-      isSticky(el, 0, 0);
+  afterEach(() => {
+    el.parentNode.remove();
+  });
+
+  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 not stuck', () => {
-      el.offsetTop = 10;
-      isSticky(el, 0, 0);
+    it('adds is-stuck class', () => {
+      isSticky(el, 0, el.offsetTop);
 
       expect(
-        el.classList.remove,
-      ).toHaveBeenCalledWith('is-stuck');
+        el.classList.contains('is-stuck'),
+      ).toBeTruthy();
+    });
+
+    it('inserts placeholder element', () => {
+      isSticky(el, 0, el.offsetTop, true);
+
+      expect(
+        document.querySelector('.sticky-placeholder'),
+      ).not.toBeNull();
     });
   });
 
-  describe('classList.add', () => {
-    it('calls classList.add when stuck', () => {
+  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.add,
+        el.classList.remove,
       ).toHaveBeenCalledWith('is-stuck');
+      expect(
+        el.classList.contains('is-stuck'),
+      ).toBeFalsy();
     });
 
-    it('does not call classList.add when not stuck', () => {
-      el.offsetTop = 10;
+    it('does not add is-stuck class', () => {
       isSticky(el, 0, 0);
 
       expect(
-        el.classList.add,
-      ).not.toHaveBeenCalled();
+        el.classList.contains('is-stuck'),
+      ).toBeFalsy();
+    });
+
+    it('removes placeholder', () => {
+      isSticky(el, 0, el.offsetTop, true);
+      isSticky(el, 0, 0, true);
+
+      expect(
+        document.querySelector('.sticky-placeholder'),
+      ).toBeNull();
     });
   });
 });