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

dleroux · 2020-01-03T14:21:47Z

WHY are these changes introduced?

WHAT is this pull request doing?

From what I can tell the issue is that the Scrollable scrollbars appear inside the Popover after the PositionOverlay has done its calculations and since the content is responsive, once the scrollbars are added the content wraps.

For example. If the window is 1000px wide, the overlay 100px wide, the left position will be calculated at 900px. Once the scrollbars appear, the left position does not get recalculated because the content is responsive. The left position pushed the overlay against the right side of the browser and when too close to the chrome causes the content to wrap. This occurs only when the popover is too close to the right of the window.

This is one of a few attempts at fixing this issue. When a popover has a preferred right, we go from the right instead of the left. That way, even if scrollbars appear, the right position stays the same, and the left does not counter it.

It also contains a change that when content can be scrolled in a Scrollable container, it sets overflow-y: scroll then reverts to auto when the content cannot be scrolled. This forces the layout engines to set the element width properly.

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

Copy-paste this code in playground/Playground.tsx:

import React, {useState} from 'react';
import {ArrowLeftMinor} from '@shopify/polaris-icons';
import {
  AppProvider,
  Frame,
  Page,
  Card,
  ActionList,
  TopBar,
  Button,
  Popover,
} from '../src';

export function Playground() {
  const [userMenuOpen, setUserMenuOpen] = useState(false);
  const [searchActive, setSearchActive] = useState(false);
  const [searchText, setSearchText] = useState('');
  const [popover1Active, setPopover1Active] = useState(false);
  const [popover2Active, setPopover2Active] = useState(false);
  const [popover3Active, setPopover3Active] = useState(false);
  const [popover1PositionBelow, setPopover1PositionBelow] = useState(true);
  const [popover2PositionBelow, setPopover2PositionBelow] = useState(true);
  const [popover3PositionBelow, setPopover3PositionBelow] = useState(true);
  const [showScrollingUserMenu, setShowScrollingUserMenu] = useState(true);

  const toggleUserMenu = () => {
    setUserMenuOpen(!userMenuOpen);
  };

  const toggleScrollingUserMenu = () => {
    setShowScrollingUserMenu(!showScrollingUserMenu);
  };

  const togglePopover = (popoverID: string) => () => {
    setPopover1Active(popoverID === 'popover1' ? !popover1Active : false);
    setPopover2Active(popoverID === 'popover2' ? !popover2Active : false);
    setPopover3Active(popoverID === 'popover3' ? !popover3Active : false);
  };

  const handleSearchResultsDismiss = () => {
    setSearchActive(false);
    setSearchText('');
  };

  const handleSearchChange = (value) => {
    setSearchText(value);
    if (value.length > 0) {
      setSearchActive(true);
    } else {
      setSearchActive(false);
    }
  };

  const theme = {
    colors: {
      topBar: {
        background: '#357997',
        backgroundLighter: '#6192a9',
        color: '#FFFFFF',
      },
    },
    logo: {
      width: 124,
      topBarSource:
        'https://cdn.shopify.com/s/files/1/0446/6937/files/jaded-pixel-logo-color.svg?6215648040070010999',
      url: 'http://jadedpixel.com',
      accessibilityLabel: 'Jaded Pixel',
    },
  };

  const actionsWithScroll = [
    {
      items: [{content: 'Back to Shopify', icon: ArrowLeftMinor}],
    },
    {
      items: [{content: 'Community forums'}],
    },
    {
      items: [{content: 'Community forums'}],
    },
    {
      items: [{content: 'Community forums'}],
    },
    {
      items: [{content: 'Community forums'}],
    },
    {
      items: [{content: 'Community forums'}],
    },
    {
      items: [{content: 'Community forums'}],
    },
  ];

  const actions = [
    {
      items: [{content: 'Back to Shopify', icon: ArrowLeftMinor}],
    },
    {
      items: [{content: 'Community forums'}],
    },
    {
      items: [{content: 'Community forums'}],
    },
    {
      items: [{content: 'Community forums'}],
    },
  ];

  const userMenuMarkup = (
    <TopBar.UserMenu
      actions={showScrollingUserMenu ? actionsWithScroll : actions}
      name="Dharma"
      detail="Jaded Pixel"
      initials="D"
      open={userMenuOpen}
      onToggle={toggleUserMenu}
    />
  );

  const searchResultsMarkup = (
    <Card>
      <ActionList
        items={[
          {content: 'Shopify help center'},
          {content: 'Community forums'},
        ]}
      />
    </Card>
  );

  const searchFieldMarkup = (
    <TopBar.SearchField
      onChange={handleSearchChange}
      value={searchText}
      placeholder="Search"
    />
  );

  const topBarMarkup = (
    <TopBar
      showNavigationToggle
      userMenu={userMenuMarkup}
      searchResultsVisible={searchActive}
      searchField={searchFieldMarkup}
      searchResults={searchResultsMarkup}
      onSearchResultsDismiss={handleSearchResultsDismiss}
    />
  );

  const activator1 = (
    <Button onClick={togglePopover('popover1')}>Popover Left</Button>
  );

  const activator2 = (
    <Button onClick={togglePopover('popover2')}>Popover Center</Button>
  );

  const activator3 = (
    <Button onClick={togglePopover('popover3')}>Popover Right</Button>
  );

  return (
    <AppProvider i18n={{}} theme={theme}>
      <Frame topBar={topBarMarkup}>
        <Page
          title="Popover alignment/width bug"
          primaryAction={{
            content: `Render ${
              showScrollingUserMenu ? 'non-scrolling' : 'scrolling'
            } user menu`,
            onAction: toggleScrollingUserMenu,
          }}
        >
          <Card>
            <Card.Section
              actions={[
                {
                  content: 'Toggle position',
                  onAction: () =>
                    setPopover1PositionBelow(!popover1PositionBelow),
                },
              ]}
            >
              <Popover
                active={popover1Active}
                activator={activator1}
                onClose={togglePopover('popover1')}
                preferredAlignment="left"
                preferredPosition={popover1PositionBelow ? 'below' : 'above'}
              >
                <ActionList
                  items={[{content: 'Import'}, {content: 'Export'}]}
                />
              </Popover>
            </Card.Section>
            <Card.Section
              actions={[
                {
                  content: 'Toggle position',
                  onAction: () =>
                    setPopover2PositionBelow(!popover2PositionBelow),
                },
              ]}
            >
              <Popover
                active={popover2Active}
                activator={activator2}
                onClose={togglePopover('popover2')}
                preferredAlignment="center"
                preferredPosition={popover2PositionBelow ? 'below' : 'above'}
              >
                <ActionList
                  items={[{content: 'Import'}, {content: 'Export'}]}
                />
              </Popover>
            </Card.Section>
            <Card.Section
              actions={[
                {
                  content: 'Toggle position',
                  onAction: () =>
                    setPopover3PositionBelow(!popover3PositionBelow),
                },
              ]}
            >
              <Popover
                active={popover3Active}
                activator={activator3}
                onClose={togglePopover('popover3')}
                preferredAlignment="right"
                preferredPosition={popover3PositionBelow ? 'below' : 'above'}
              >
                <ActionList
                  items={[{content: 'Import'}, {content: 'Export'}]}
                />
              </Popover>
            </Card.Section>
          </Card>
        </Page>
      </Frame>
    </AppProvider>
  );
}

🎩 checklist

Tested on mobile
Tested on multiple browsers
Tested for accessibility
Updated the component's README.md with documentation changes
Tophatted documentation changes in the style guide
For visual design changes, pinged one of @ HYPD, @ mirualves, @ sarahill, or @ ry5n to update the Polaris UI kit

dleroux · 2020-01-03T14:22:55Z

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;


Not sure why we needed to remove the right margin when positioned above.

github-actions · 2020-01-03T14:24:04Z

💦 Potential splash zone of changes introduced to src/**/*.tsx in this pull request:

Files modified	8
Files potentially affected	42

Details

All files potentially affected (total: 42)

📄 UNRELEASED.md (total: 0)

Files potentially affected (total: 0)

🎨 src/components/Popover/Popover.scss (total: 25)

Files potentially affected (total: 25)

🧩 src/components/PositionedOverlay/PositionedOverlay.tsx (total: 28)

Files potentially affected (total: 28)

🧩 src/components/PositionedOverlay/tests/PositionedOverlay.test.tsx (total: 0)

Files potentially affected (total: 0)

🧩 src/components/PositionedOverlay/utilities/math.ts (total: 29)

Files potentially affected (total: 29)

🎨 src/components/Scrollable/Scrollable.scss (total: 42)

Files potentially affected (total: 42)

🧩 src/components/Scrollable/Scrollable.tsx (total: 41)

Files potentially affected (total: 41)

🧩 src/components/TopBar/components/Menu/Menu.tsx (total: 2)

Files potentially affected (total: 2)

This comment automatically updates as changes are made to this pull request.
Feedback, troubleshooting: open an issue or reach out on Slack in #polaris-tooling.

dleroux · 2020-01-03T14:24:25Z

src/components/PositionedOverlay/PositionedOverlay.tsx

-            left: horizontalPosition,
+            left:
+              preferredAlignment !== 'right' ? horizontalPosition : undefined,
+            right:


We're only going to go from the right if the preferred alignment is right.

dleroux · 2020-01-03T14:25:49Z

src/components/PositionedOverlay/PositionedOverlay.tsx

@@ -298,7 +317,7 @@ function windowRect() {
    top: window.scrollY,
    left: window.scrollX,
    height: window.innerHeight,
-    width: window.innerWidth,
+    width: document.body.clientWidth,


window.innerWidth includes the scrollbars, We don't want this when aligning from the right and it's shouldn't impact the left.

TIL! Will this change alone fix the measuring then and the other stuff in this PR is just a nice to have?

No, the right positioning is what is needed. I'll write it in the description.

dleroux · 2020-01-03T14:26:25Z

src/components/PositionedOverlay/utilities/math.ts

@@ -98,13 +98,11 @@ 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);


the right position calculation.

dleroux · 2020-01-03T14:26:57Z

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

@@ -57,6 +57,7 @@ export function Menu(props: MenuProps) {
      onClose={onClose}
      fixed
      fullHeight={isFullHeight}
+      preferredAlignment="right"


The user menu was causing the issue. This fixed it.

dleroux · 2020-01-03T16:35:22Z

src/components/PositionedOverlay/PositionedOverlay.tsx

-    horizontal: parseFloat(nodeStyles.marginLeft || ''),
+    activator: parseFloat(nodeStyles.marginTop || '0'),
+    container: parseFloat(nodeStyles.marginBottom || '0'),
+    horizontal: parseFloat(nodeStyles.marginLeft || '0'),


defaulting to 0. an empty string didn't make sense for the tests.

dleroux · 2020-01-03T16:36:33Z

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

+
+      expect((positionedOverlay.find('div').prop('style') as any).right).toBe(
+        0,
+      );
      expect(
        (positionedOverlay.find('div').prop('style') as any).left,


This test didn't make sense. No matter whar left would be undefined, which it shouldn't be.

UNRELEASED.md

dleroux · 2020-01-03T18:27:57Z

:( Just saw that safari still wraps for no good reason.

dleroux · 2020-01-06T13:08:15Z

Looks like this approach works perfectly on Chrome, but Firefox and Safari do not resize the parent PositionOverlay when the scrollbars appear. I was hoping a simple CSS change would fix it but I was unsuccessful.

chloerice · 2020-01-08T21:46:33Z

Looks like this approach works perfectly on Chrome, but Firefox and Safari do not resize the parent PositionOverlay when the scrollbars appear. I was hoping a simple CSS change would fix it but I was unsuccessful.

Yeah, been there with these changes 😞 Every approach we tried was inconsistent across browsers. Only idea I have left is to start from scratch with this component... I feel like a lot of the logic is unnecessary, but because it's unclear what some of it is meant to do, refactoring it while trying to fix this just kept breaking it in other ways 😣.

dleroux · 2020-01-08T22:23:41Z

@chloerice I thought of redoing it but after wrapping my head around it it's actually pretty good and I fear we'll end up in the same spot. The only thing I can think of is to add complexity and measure whether or not the content will cause scrollbars, then measure the scrollbar, then add the scrollbar width to the container. The math is all correct.

I asked @tmlayton to take a look and see if he can see something we're missing.

It works perfectly in Chrome right now. It's almost tempting to ship this.

AndrewMusgrave · 2020-01-10T17:48:54Z

It's almost tempting to ship this.

We could always revert this when we have a better fix. Getting something out that'll work for the majority of consumers seems like a win to me.

alex-page · 2020-01-10T17:53:47Z

I made this PR #2362 that starts the logic again from scratch, removing the confusing Math.max. This was working in all browsers last time I tested but it was a bigger code change and higher risk.

As Tim mentioned it needs tests to make sure it doesn't break anything.

tmlayton · 2020-01-11T00:58:18Z

Alright, this should be good now. When content can be scrolled in a Scrollable container, it sets overflow-y: scroll and reverts to auto when the content cannot be scrolled. This forces the layout engines to set the element width properly. Here are screenshots from Chrome, FF, Safari, and Edge with and without scrollbars:

Always on scrollbars

Always on scrollbars with non-scrolling content

Floating scrollbars (does not apply to Edge)

tmlayton

👍

kyledurand

🌟 🙌

dleroux · 2020-01-20T13:54:30Z

@tmlayton you are a 🌟 🙇

dleroux commented Jan 3, 2020

View reviewed changes

dleroux requested a review from kyledurand January 3, 2020 15:13

dleroux changed the title ~~[WIP][Popover] Set right position when preferred alignment is right.~~ [Popover] Set right position when preferred alignment is right. Jan 3, 2020

dleroux changed the title ~~[Popover] Set right position when preferred alignment is right.~~ [wip][Popover] Set right position when preferred alignment is right. Jan 3, 2020

dleroux force-pushed the popover-position-fix branch from 121e07d to 782d7b1 Compare January 3, 2020 16:31

dleroux changed the title ~~[wip][Popover] Set right position when preferred alignment is right.~~ [Popover] Set right position when preferred alignment is right. Jan 3, 2020

dleroux commented Jan 3, 2020

View reviewed changes

dleroux requested review from alex-page, chloerice and tmlayton January 3, 2020 16:36

dleroux commented Jan 3, 2020

View reviewed changes

UNRELEASED.md Outdated Show resolved Hide resolved

dleroux changed the title ~~[Popover] Set right position when preferred alignment is right.~~ [WIP][Popover] Set right position when preferred alignment is right. Jan 3, 2020

dleroux assigned tmlayton Jan 6, 2020

dleroux removed request for kyledurand, chloerice and alex-page January 6, 2020 13:08

dleroux and others added 2 commits January 10, 2020 15:20

added right calculation

231b77e

Update UNRELEASED.md

e819b92

Tim Layton added 2 commits January 10, 2020 16:33

remove lint comments

210ab4e

set scrollable in scroll mode to render element width properly

a3998fa

tmlayton force-pushed the popover-position-fix branch from 5db1747 to a3998fa Compare January 11, 2020 00:34

Tim Layton added 2 commits January 10, 2020 16:42

remove duplicate changelog entry

771a635

use css class instead of inline styles

7339c8d

tmlayton changed the title ~~[WIP][Popover] Set right position when preferred alignment is right.~~ [Popover] Set right position when preferred alignment is right. Jan 11, 2020

tmlayton approved these changes Jan 11, 2020

View reviewed changes

kyledurand approved these changes Jan 11, 2020

View reviewed changes

tmlayton merged commit ffd3ed5 into master Jan 13, 2020

tmlayton deleted the popover-position-fix branch January 13, 2020 18:26

alex-page mentioned this pull request Jan 13, 2020

[WIP] Fix calculation for center aligned popover #2362

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

dleroux commented Jan 3, 2020 •

edited by tmlayton

Loading

dleroux Jan 3, 2020

github-actions bot commented Jan 3, 2020 •

edited

Loading

Files potentially affected (total: 0)

Files potentially affected (total: 25)

Files potentially affected (total: 28)

Files potentially affected (total: 0)

Files potentially affected (total: 29)

Files potentially affected (total: 42)

Files potentially affected (total: 41)

Files potentially affected (total: 2)

dleroux Jan 3, 2020 •

edited

Loading

dleroux Jan 3, 2020

kyledurand Jan 3, 2020

dleroux Jan 3, 2020

dleroux Jan 3, 2020

dleroux Jan 3, 2020

dleroux Jan 3, 2020

dleroux Jan 3, 2020

dleroux commented Jan 3, 2020

dleroux commented Jan 6, 2020

chloerice commented Jan 8, 2020 •

edited

Loading

dleroux commented Jan 8, 2020

AndrewMusgrave commented Jan 10, 2020

alex-page commented Jan 10, 2020 •

edited

Loading

tmlayton commented Jan 11, 2020

tmlayton left a comment

kyledurand left a comment

dleroux commented Jan 20, 2020

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

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

Conversation

dleroux commented Jan 3, 2020 • edited by tmlayton Loading

WHY are these changes introduced?

WHAT is this pull request doing?

How to 🎩

🎩 checklist

Choose a reason for hiding this comment

github-actions bot commented Jan 3, 2020 • edited Loading

Details

Files potentially affected (total: 0)

Files potentially affected (total: 25)

Files potentially affected (total: 28)

Files potentially affected (total: 0)

Files potentially affected (total: 29)

Files potentially affected (total: 42)

Files potentially affected (total: 41)

Files potentially affected (total: 2)

dleroux Jan 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dleroux commented Jan 3, 2020

dleroux commented Jan 6, 2020

chloerice commented Jan 8, 2020 • edited Loading

dleroux commented Jan 8, 2020

AndrewMusgrave commented Jan 10, 2020

alex-page commented Jan 10, 2020 • edited Loading

tmlayton commented Jan 11, 2020

Always on scrollbars

Always on scrollbars with non-scrolling content

Floating scrollbars (does not apply to Edge)

tmlayton left a comment

Choose a reason for hiding this comment

kyledurand left a comment

Choose a reason for hiding this comment

dleroux commented Jan 20, 2020

dleroux commented Jan 3, 2020 •

edited by tmlayton

Loading

github-actions bot commented Jan 3, 2020 •

edited

Loading

dleroux Jan 3, 2020 •

edited

Loading

chloerice commented Jan 8, 2020 •

edited

Loading

alex-page commented Jan 10, 2020 •

edited

Loading