Proper margin orientation for scroll into view (#17)

primer · Dec 3, 2021 · 9194ba4 · 9194ba4
1 parent 4062144
commit 9194ba4
Show file tree

Hide file tree

Showing 4 changed files with 12 additions and 7 deletions.
diff --git a/.changeset/sixty-months-accept.md b/.changeset/sixty-months-accept.md
@@ -0,0 +1,5 @@
+---
+'@primer/behaviors': patch
+---
+
+Correct margin orientation for `scrollIntoView`
diff --git a/docs/scroll-into-view.md b/docs/scroll-into-view.md
@@ -43,6 +43,6 @@ The `scrollIntoView` function takes the following arguments.
 | Name        | Type                        | Default      | Description                                                                                                                                                      |
 | :---------- | :-------------------------- | :----------- | :--------------------------------------------------------------------------------------------------------------------------------------------------------------- |
 | direction   | `'horizontal' │ 'vertical'` | `'vertical'` | The direction to scroll the viewing area                                                                                                                         |
-| startMargin | `number` of pixels          | 8            | A margin to leave between the start of the viewing area and the start of the child element (if room allows)                                                      |
+| startMargin | `number` of pixels          | 0            | A margin to leave between the start of the viewing area and the start of the child element (if room allows)                                                      |
 | endMargin   | `number` of pixels          | 0            | A margin to leave between the end of the viewing area and the end of the child element (if room allows)                                                          |
 | behavior    | `ScrollBehavior`            | `'smooth'`   | Behavior for the browser to use when scrolling.  `smooth` will transition with a smooth animation, while `auto` will immediately jump to the new scroll location |
diff --git a/src/__tests__/scroll-into-view.test.ts b/src/__tests__/scroll-into-view.test.ts
@@ -51,7 +51,7 @@ describe('scrollIntoView', () => {
     const viewAreaHeight = 100
     const childStart = viewAreaHeight + 10
     const expectedScrollPosition = scrollPositionFormula(
-      {viewingAreaEdgePosition: viewAreaHeight, childEdgePosition: childStart + childHeight, margin: 8},
+      {viewingAreaEdgePosition: viewAreaHeight, childEdgePosition: childStart + childHeight, margin: 0},
       false
     )
 

diff --git a/src/scroll-into-view.ts b/src/scroll-into-view.ts
@@ -8,23 +8,23 @@ export interface ScrollIntoViewOptions {
 export function scrollIntoView(
   child: HTMLElement,
   viewingArea: HTMLElement,
-  {direction = 'vertical', startMargin = 8, endMargin = 0, behavior = 'smooth'}: ScrollIntoViewOptions = {}
+  {direction = 'vertical', startMargin = 0, endMargin = 0, behavior = 'smooth'}: ScrollIntoViewOptions = {}
 ) {
   const startSide = direction === 'vertical' ? 'top' : 'left'
   const endSide = direction === 'vertical' ? 'bottom' : 'right'
   const scrollSide = direction === 'vertical' ? 'scrollTop' : 'scrollLeft'
   const {[startSide]: childStart, [endSide]: childEnd} = child.getBoundingClientRect()
   const {[startSide]: viewingAreaStart, [endSide]: viewingAreaEnd} = viewingArea.getBoundingClientRect()
 
-  const isChildStartAboveViewingArea = childStart < viewingAreaStart + endMargin
-  const isChildBottomBelowViewingArea = childEnd > viewingAreaEnd - startMargin
+  const isChildStartAboveViewingArea = childStart < viewingAreaStart + startMargin
+  const isChildBottomBelowViewingArea = childEnd > viewingAreaEnd - endMargin
 
   if (isChildStartAboveViewingArea) {
     const scrollHeightToChildStart = childStart - viewingAreaStart + viewingArea[scrollSide]
-    viewingArea.scrollTo({behavior, [startSide]: scrollHeightToChildStart - endMargin})
+    viewingArea.scrollTo({behavior, [startSide]: scrollHeightToChildStart - startMargin})
   } else if (isChildBottomBelowViewingArea) {
     const scrollHeightToChildBottom = childEnd - viewingAreaEnd + viewingArea[scrollSide]
-    viewingArea.scrollTo({behavior, [startSide]: scrollHeightToChildBottom + startMargin})
+    viewingArea.scrollTo({behavior, [startSide]: scrollHeightToChildBottom + endMargin})
   }
 
   // either completely in view or outside viewing area on both ends, don't scroll