fix(Popup): allow to 'detach' from trigger and RTL adjustments (#612)

* introduce offset prop

* correct description of supported values

* update changelog

* introduce fix

* ensure RTL is properly applied to complex offset expressions

* rename method to make logic more expressive

* add unit tests

* remove unnecessary grid props from offset example

* update changelog

CHANGELOG.md

@@ -17,6 +17,9 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 
 ## [Unreleased]
 
+### Fixes
+- Ensure `Popup` properly flips values of `offset` prop in RTL @kuzhelov ([#612](https://github.com/stardust-ui/react/pull/612))
+
 ### Features
 - Add `color` prop to `Text` component @Bugaa92 ([#597](https://github.com/stardust-ui/react/pull/597))
 - Add `color` prop to `Header` and `HeaderDescription` components @Bugaa92 ([#628](https://github.com/stardust-ui/react/pull/628))

docs/src/examples/components/Popup/Variations/PopupExampleOffset.shorthand.tsx

@@ -1,5 +1,5 @@
 import React from 'react'
-import { Button, Grid, Popup, Alignment, Position } from '@stardust-ui/react'
+import { Button, Grid, Popup } from '@stardust-ui/react'
 
 const renderButton = rotateArrowUp => (
   <Button
@@ -11,33 +11,24 @@ const renderButton = rotateArrowUp => (
   />
 )
 
-const triggers = [
-  { position: 'above', align: 'start', offset: '-100%p', rotateArrowUp: '-45deg' },
-  { position: 'above', align: 'end', offset: '100%p', rotateArrowUp: '45deg' },
-  { position: 'below', align: 'start', offset: '-100%p', rotateArrowUp: '-135deg' },
-  { position: 'below', align: 'end', offset: '100%p', rotateArrowUp: '135deg' },
-]
-
 const PopupExamplePosition = () => (
-  <Grid columns="repeat(2, 80px)" variables={{ padding: '30px', gridGap: '30px' }}>
-    {triggers.map(({ position, align, offset, rotateArrowUp }) => (
-      <Popup
-        align={align as Alignment}
-        position={position as Position}
-        offset={offset}
-        trigger={renderButton(rotateArrowUp)}
-        content={{
-          content: (
-            <p>
-              The popup is rendered at {position}-{align}
-              <br />
-              corner of the trigger.
-            </p>
-          ),
-        }}
-        key={`${position}-${align}`}
-      />
-    ))}
+  <Grid columns="1, 80px" variables={{ padding: '30px' }}>
+    <Popup
+      align="start"
+      position="above"
+      offset="-100%p"
+      trigger={renderButton('-45deg')}
+      content={{
+        content: (
+          <p>
+            The popup is rendered at above-start
+            <br />
+            corner of the trigger.
+          </p>
+        ),
+      }}
+      key="above-start"
+    />
   </Grid>
 )
 

src/components/Popup/Popup.tsx

@@ -18,7 +18,7 @@ import {
 import { ComponentEventHandler, Extendable, ShorthandValue } from '../../../types/utils'
 
 import Ref from '../Ref/Ref'
-import computePopupPlacement, { Alignment, Position } from './positioningHelper'
+import { getPopupPlacement, applyRtlToOffset, Alignment, Position } from './positioningHelper'
 
 import PopupContent from './PopupContent'
 
@@ -257,11 +257,14 @@ export default class Popup extends AutoControlledComponent<Extendable<PopupProps
     const { align, position, offset } = this.props
     const { target } = this.state
 
-    const placement = computePopupPlacement({ align, position, rtl })
+    const placement = getPopupPlacement({ align, position, rtl })
 
     const popperModifiers = {
       // https://popper.js.org/popper-documentation.html#modifiers..offset
-      ...(offset && { offset: { offset: this.applyRtlToOffset(offset, rtl, position) } }),
+      ...(offset && {
+        offset: { offset: rtl ? applyRtlToOffset(offset, position) : offset },
+        keepTogether: { enabled: false },
+      }),
     }
 
     return (
@@ -331,14 +334,4 @@ export default class Popup extends AutoControlledComponent<Extendable<PopupProps
       _.invoke(this.props, 'onOpenChange', eventArgs, { ...this.props, ...{ open: newValue } })
     }
   }
-
-  private applyRtlToOffset(offset: string, rtl: boolean, position: Position): string {
-    if (rtl && (position === 'above' || position === 'below')) {
-      return offset.trimLeft().startsWith('-')
-        ? offset.trimLeft().replace(/^-\s*/, '')
-        : `-${offset}`
-    }
-
-    return offset
-  }
 }

src/components/Popup/positioningHelper.ts

@@ -56,7 +56,7 @@ const shouldAlignToCenter = (p: Position, a: Alignment) => {
  * | after    | center    |  right          |  left
  * | after    | bottom    |  right-end      |  left-end
  */
-export default ({
+export const getPopupPlacement = ({
   align,
   position,
   rtl,
@@ -74,3 +74,30 @@ export default ({
 
   return `${computedPosition}${stringifiedAlignment}` as Placement
 }
+
+/////////////////////////////////
+// OFFSET VALUES ADJUSTMENT
+/////////////////////////////////
+
+const flipPlusMinusSigns = (offset: string): string => {
+  return offset
+    .replace(/\-/g, '<plus>')
+    .replace(/^(\s*)(?=\d)/, '<minus>')
+    .replace(/\+/g, '<minus>')
+    .replace(/<plus>/g, '+')
+    .replace(/<minus>/g, '-')
+    .trimLeft()
+    .replace(/^\+/, '')
+}
+
+export const applyRtlToOffset = (offset: string, position: Position): string => {
+  if (position === 'above' || position === 'below') {
+    const [horizontal, vertical] = offset.split(',')
+    return [flipPlusMinusSigns(horizontal), vertical]
+      .join(', ')
+      .replace(/, $/, '')
+      .trim()
+  }
+
+  return offset
+}

test/specs/components/Popup/Popup-test.tsx

@@ -1,4 +1,9 @@
-import computePopupPlacement, { Position, Alignment } from 'src/components/Popup/positioningHelper'
+import {
+  getPopupPlacement,
+  applyRtlToOffset,
+  Position,
+  Alignment,
+} from 'src/components/Popup/positioningHelper'
 import { Placement } from 'popper.js'
 
 type PositionTestInput = {
@@ -16,7 +21,7 @@ describe('Popup', () => {
     rtl = false,
   }: PositionTestInput) =>
     it(`Popup ${position} position is transformed to ${expectedPlacement} Popper's placement`, () => {
-      const actualPlacement = computePopupPlacement({ align, position, rtl })
+      const actualPlacement = getPopupPlacement({ align, position, rtl })
       expect(actualPlacement).toEqual(expectedPlacement)
     })
 
@@ -56,4 +61,45 @@ describe('Popup', () => {
     testPopupPositionInRtl({ position: 'after', align: 'center', expectedPlacement: 'left' })
     testPopupPositionInRtl({ position: 'after', align: 'bottom', expectedPlacement: 'left-end' })
   })
+
+  describe('Popup offset transformed correctly in RTL', () => {
+    it("applies transform only for 'above' and 'below' postioning", () => {
+      const originalOffsetValue = '100%'
+
+      expect(applyRtlToOffset(originalOffsetValue, 'above')).not.toBe(originalOffsetValue)
+      expect(applyRtlToOffset(originalOffsetValue, 'below')).not.toBe(originalOffsetValue)
+
+      expect(applyRtlToOffset(originalOffsetValue, 'before')).toBe(originalOffsetValue)
+      expect(applyRtlToOffset(originalOffsetValue, 'after')).toBe(originalOffsetValue)
+    })
+
+    const expectOffsetTransformResult = (originalOffset, resultOffset) => {
+      expect(applyRtlToOffset(originalOffset, 'above')).toBe(resultOffset)
+    }
+
+    it('flips sign of simple expressions', () => {
+      expectOffsetTransformResult('100%', '-100%')
+      expectOffsetTransformResult('  2000%p ', '-2000%p')
+      expectOffsetTransformResult('100  ', '-100')
+      expectOffsetTransformResult(' - 200vh', '200vh')
+    })
+
+    it('flips sign of complex expressions', () => {
+      expectOffsetTransformResult('100% + 200', '-100% - 200')
+      expectOffsetTransformResult(' - 2000%p - 400 +800vh ', '2000%p + 400 -800vh')
+    })
+
+    it('transforms only horizontal offset value', () => {
+      const xOffset = '-100%'
+      const yOffset = '800vh'
+
+      const offsetValue = [xOffset, yOffset].join(',')
+      const [xOffsetTransformed, yOffsetTransformed] = applyRtlToOffset(offsetValue, 'above').split(
+        ',',
+      )
+
+      expect(xOffsetTransformed.trim()).not.toBe(xOffset)
+      expect(yOffsetTransformed.trim()).toBe(yOffset)
+    })
+  })
 })