[Popover] Set right position when preferred alignment is right.

UNRELEASED.md

@@ -21,6 +21,7 @@
 
 - Fixed an issue with the `Filters` component where the `aria-expanded` attribute was `undefined` on mount ([#2589]https://github.com/Shopify/polaris-react/pull/2589)
 - Fixed `TrapFocus` from tabbing out of the container ([#2555](https://github.com/Shopify/polaris-react/pull/2555))
+- Fixed `PositionedOverlay` not correctly getting its position when aligned to the right of the activator ([#2587](https://github.com/Shopify/polaris-react/pull/2587))
 
 ### Documentation
 

src/components/Popover/Popover.scss

@@ -52,7 +52,7 @@ $content-max-width: rem(400px);
 }
 
 .positionedAbove {
-  margin: spacing() 0 $visible-portion-of-arrow spacing(tight);
+  margin: spacing() spacing(tight) $visible-portion-of-arrow;
 
   &.fullWidth {
     margin: 0 0 $visible-portion-of-arrow;

src/components/PositionedOverlay/PositionedOverlay.tsx

@@ -22,7 +22,8 @@ export {PreferredPosition, PreferredAlignment};
 export type Positioning = 'above' | 'below';
 
 export interface OverlayDetails {
-  left: number;
+  left?: number;
+  right?: number;
   desiredHeight: number;
   positioning: Positioning;
   measuring: boolean;
@@ -44,7 +45,8 @@ export interface PositionedOverlayProps {
 interface State {
   measuring: boolean;
   activatorRect: Rect;
-  left: number;
+  left?: number;
+  right?: number;
   top: number;
   height: number;
   width: number | null;
@@ -63,7 +65,8 @@ export class PositionedOverlay extends React.PureComponent<
   state: State = {
     measuring: true,
     activatorRect: getRectForNode(this.props.activator),
-    left: 0,
+    right: undefined,
+    left: undefined,
     top: 0,
     height: 0,
     width: null,
@@ -118,12 +121,13 @@ export class PositionedOverlay extends React.PureComponent<
   }
 
   render() {
-    const {left, top, zIndex, width} = this.state;
+    const {left, right, top, zIndex, width} = this.state;
     const {render, fixed, classNames: propClassNames} = this.props;
 
     const style = {
       top: top == null || isNaN(top) ? undefined : top,
       left: left == null || isNaN(left) ? undefined : left,
+      right: right == null || isNaN(right) ? undefined : right,
       width: width == null || isNaN(width) ? undefined : width,
       zIndex: zIndex == null || isNaN(zIndex) ? undefined : zIndex,
     };
@@ -143,11 +147,19 @@ export class PositionedOverlay extends React.PureComponent<
   }
 
   private overlayDetails = (): OverlayDetails => {
-    const {measuring, left, positioning, height, activatorRect} = this.state;
+    const {
+      measuring,
+      left,
+      right,
+      positioning,
+      height,
+      activatorRect,
+    } = this.state;
 
     return {
       measuring,
       left,
+      right,
       desiredHeight: height,
       positioning,
       activatorRect,
@@ -164,8 +176,9 @@ export class PositionedOverlay extends React.PureComponent<
     this.observer.disconnect();
 
     this.setState(
-      ({left, top}) => ({
+      ({left, top, right}) => ({
         left,
+        right,
         top,
         height: 0,
         positioning: 'below',
@@ -209,6 +222,7 @@ export class PositionedOverlay extends React.PureComponent<
         const overlayMargins = this.overlay.firstElementChild
           ? getMarginsForNode(this.overlay.firstElementChild as HTMLElement)
           : {activator: 0, container: 0, horizontal: 0};
+
         const containerRect = windowRect();
         const zIndexForLayer = getZIndexForLayerFromNode(activator);
         const zIndex =
@@ -234,7 +248,10 @@ export class PositionedOverlay extends React.PureComponent<
           {
             measuring: false,
             activatorRect: getRectForNode(activator),
-            left: horizontalPosition,
+            left:
+              preferredAlignment !== 'right' ? horizontalPosition : undefined,
+            right:
+              preferredAlignment === 'right' ? horizontalPosition : undefined,
             top: lockPosition ? top : verticalPosition.top,
             lockPosition: Boolean(fixed),
             height: verticalPosition.height || 0,
@@ -278,9 +295,9 @@ export function intersectionWithViewport(
 function getMarginsForNode(node: HTMLElement) {
   const nodeStyles = window.getComputedStyle(node);
   return {
-    activator: parseFloat(nodeStyles.marginTop || ''),
-    container: parseFloat(nodeStyles.marginBottom || ''),
-    horizontal: parseFloat(nodeStyles.marginLeft || ''),
+    activator: parseFloat(nodeStyles.marginTop || '0'),
+    container: parseFloat(nodeStyles.marginBottom || '0'),
+    horizontal: parseFloat(nodeStyles.marginLeft || '0'),
   };
 }
 
@@ -298,7 +315,7 @@ function windowRect() {
     top: window.scrollY,
     left: window.scrollX,
     height: window.innerHeight,
-    width: window.innerWidth,
+    width: document.body.clientWidth,
   });
 }
 

src/components/PositionedOverlay/tests/PositionedOverlay.test.tsx

@@ -70,6 +70,20 @@ describe('<PositionedOverlay />', () => {
         <PositionedOverlay {...mockProps} preferredAlignment="left" />,
       );
 
+      expect((positionedOverlay.find('div').prop('style') as any).left).toBe(0);
+      expect(
+        (positionedOverlay.find('div').prop('style') as any).right,
+      ).toBeUndefined();
+    });
+
+    it('aligns right if preferredAlignment is given', () => {
+      const positionedOverlay = mountWithAppProvider(
+        <PositionedOverlay {...mockProps} preferredAlignment="right" />,
+      );
+
+      expect((positionedOverlay.find('div').prop('style') as any).right).toBe(
+        0,
+      );
       expect(
         (positionedOverlay.find('div').prop('style') as any).left,
       ).toBeUndefined();

src/components/PositionedOverlay/utilities/math.ts

@@ -98,13 +98,12 @@ export function calculateHorizontalPosition(
       Math.max(0, activatorRect.left - overlayMargins.horizontal),
     );
   } else if (preferredAlignment === 'right') {
-    const activatorRight = activatorRect.left + activatorRect.width;
+    const activatorRight =
+      containerRect.width - (activatorRect.left + activatorRect.width);
+
     return Math.min(
       maximum,
-      Math.max(
-        0,
-        activatorRight - overlayRect.width + overlayMargins.horizontal,
-      ),
+      Math.max(0, activatorRight - overlayMargins.horizontal),
     );
   }
 

src/components/Scrollable/Scrollable.scss

@@ -20,6 +20,10 @@ $shadow-top: inset 0 $shadow-size $shadow-size (-1 * $shadow-size)
   overflow-y: auto;
 }
 
+.verticalHasScrolling {
+  overflow-y: scroll;
+}
+
 .hasTopShadow {
   box-shadow: $shadow-top;
 }

src/components/Scrollable/Scrollable.tsx

@@ -42,6 +42,7 @@ interface State {
   topShadow: boolean;
   bottomShadow: boolean;
   scrollPosition: number;
+  canScroll: boolean;
 }
 
 export class Scrollable extends React.Component<ScrollableProps, State> {
@@ -56,6 +57,7 @@ export class Scrollable extends React.Component<ScrollableProps, State> {
     topShadow: false,
     bottomShadow: false,
     scrollPosition: 0,
+    canScroll: false,
   };
 
   private stickyManager = new StickyManager();
@@ -104,7 +106,7 @@ export class Scrollable extends React.Component<ScrollableProps, State> {
   }
 
   render() {
-    const {topShadow, bottomShadow} = this.state;
+    const {topShadow, bottomShadow, canScroll} = this.state;
     const {
       children,
       className,
@@ -123,6 +125,7 @@ export class Scrollable extends React.Component<ScrollableProps, State> {
       horizontal && styles.horizontal,
       topShadow && styles.hasTopShadow,
       bottomShadow && styles.hasBottomShadow,
+      vertical && canScroll && styles.verticalHasScrolling,
     );
 
     return (
@@ -168,6 +171,7 @@ export class Scrollable extends React.Component<ScrollableProps, State> {
       topShadow: shouldTopShadow,
       bottomShadow: shouldBottomShadow,
       scrollPosition: scrollTop,
+      canScroll,
     });
   };
 

src/components/TopBar/components/Menu/Menu.tsx

@@ -57,6 +57,7 @@ export function Menu(props: MenuProps) {
       onClose={onClose}
       fixed
       fullHeight={isFullHeight}
+      preferredAlignment="right"
     >
       <ActionList onActionAnyItem={onClose} sections={actions} />
       {messageMarkup}