[WIP] Fix PositionOverlay width issue for the menu in TopBar #1692

alex-page · 2019-06-17T17:02:20Z

WHY are these changes introduced?

WHAT is this pull request doing?

Using mutation observer so that the resize calculation is being run at the correct time. It is also resetting the left value to 0 which is needed for the recalc.

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

This needs to be tested in web and in Polaris with around 6-7 menu items.

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

import * as React from 'react';
import {ArrowLeftMinor} from '@shopify/polaris-icons';
import {string} from 'prop-types';
import {
  AppProvider,
  Frame,
  Page,
  Card,
  ActionList,
  TopBar,
  Button,
  Popover,
  PopoverProps,
} from '../src';

interface State {
  userMenuOpen: boolean;
  searchActive: boolean;
  searchText: string;
  popover1Active: boolean;
  popover2Active: boolean;
  popover3Active: boolean;
  popover1PositionBelow: boolean;
  popover2PositionBelow: boolean;
  popover3PositionBelow: boolean;
  showScrollingUserMenu: boolean;
}

export class Playground extends React.Component {
  state: State = {
    userMenuOpen: false,
    searchActive: false,
    searchText: '',
    popover1Active: false,
    popover2Active: false,
    popover3Active: false,
    popover1PositionBelow: true,
    popover2PositionBelow: true,
    popover3PositionBelow: true,
    showScrollingUserMenu: true,
  };

  render() {
    const {
      state,
      handleSearchChange,
      handleSearchResultsDismiss,
      toggleUserMenu,
    } = this;
    const {
      userMenuOpen,
      searchText,
      searchActive,
      showScrollingUserMenu,
    } = state;

    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={this.togglePopover('popover1', 'Active')}>
        Popover Left
      </Button>
    );

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

    const activator3 = (
      <Button onClick={this.togglePopover('popover3', 'Active')}>
        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: this.toggleScrollingUserMenu,
            }}
          >
            <Card>
              <Card.Section
                actions={[
                  {
                    content: 'Toggle position',
                    onAction: this.togglePopover('popover1', 'PositionBelow'),
                  },
                ]}
              >
                <Popover
                  active={this.state.popover1Active}
                  activator={activator1}
                  onClose={this.togglePopover('popover1', 'Active')}
                  preferredAlignment="left"
                  preferredPosition={
                    this.state.popover1PositionBelow ? 'below' : 'above'
                  }
                >
                  <ActionList
                    items={[{content: 'Import'}, {content: 'Export'}]}
                  />
                </Popover>
              </Card.Section>
              <Card.Section
                actions={[
                  {
                    content: 'Toggle position',
                    onAction: this.togglePopover('popover2', 'PositionBelow'),
                  },
                ]}
              >
                <Popover
                  active={this.state.popover2Active}
                  activator={activator2}
                  onClose={this.togglePopover('popover2', 'Active')}
                  preferredAlignment="center"
                  preferredPosition={
                    this.state.popover2PositionBelow ? 'below' : 'above'
                  }
                >
                  <ActionList
                    items={[{content: 'Import'}, {content: 'Export'}]}
                  />
                </Popover>
              </Card.Section>
              <Card.Section
                actions={[
                  {
                    content: 'Toggle position',
                    onAction: this.togglePopover('popover3', 'PositionBelow'),
                  },
                ]}
              >
                <Popover
                  active={this.state.popover3Active}
                  activator={activator3}
                  onClose={this.togglePopover('popover3', 'Active')}
                  preferredAlignment="right"
                  preferredPosition={
                    this.state.popover3PositionBelow ? 'below' : 'above'
                  }
                >
                  <ActionList
                    items={[{content: 'Import'}, {content: 'Export'}]}
                  />
                </Popover>
              </Card.Section>
            </Card>
          </Page>
        </Frame>
      </AppProvider>
    );
  }

  toggleUserMenu = () => {
    this.setState(({userMenuOpen}: State) => ({userMenuOpen: !userMenuOpen}));
  };

  toggleScrollingUserMenu = () => {
    this.setState(({showScrollingUserMenu, userMenuOpen}: State) => ({
      showScrollingUserMenu: !showScrollingUserMenu,
    }));
  };

  togglePopover = (popoverID: string, prop: string) => () => {
    this.setState((state: State) => {
      return {[`${popoverID}${prop}`]: !state[`${popoverID}${prop}`]};
    });
  };

  handleSearchResultsDismiss = () => {
    this.setState(() => {
      return {
        searchActive: false,
        searchText: '',
      };
    });
  };

  handleSearchChange = (value) => {
    this.setState({searchText: value});
    if (value.length > 0) {
      this.setState({searchActive: true});
    } else {
      this.setState({searchActive: false});
    }
  };
}

🎩 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

alex-page · 2019-06-17T17:20:44Z

@ken3650 I have added as a reviewer on this PR as it reverts some of the changes you made in #1382

@AndrewMusgrave I have added you as a reviewer as it uses your mutation observer code.

danrosenthal · 2019-06-17T19:27:02Z

Can you or @AndrewMusgrave explain about MutationObserver?

AndrewMusgrave · 2019-06-17T22:13:37Z

@danrosenthal #1621

If the change is needed for this, I'm tempted to cherry pick the commit into v3

danrosenthal · 2019-06-18T12:59:12Z

If the change is needed for this, I'm tempted to cherry pick the commit into v3

Seems like it is.

alex-page · 2019-06-18T13:07:15Z

It's not "needed" but it made debugging and performance of the component a lot better. @AndrewMusgrave please cherry pick.

danrosenthal · 2019-06-18T13:59:48Z

Looks like total code coverage could be achieved with a couple testing tweaks

👀

Testing it additionally by making fixed false ought to get us to 90%. Would it also be possible to not have an overlay in one of the tests?

ken3650

Can't speak to all the changes, but 🎩'ed the change against my earlier issue that this changes and all is good 👍

AndrewMusgrave · 2019-06-18T18:04:59Z

Would it also be possible to not have an overlay in one of the tests?

Nope since the code will never run before componentDidMount 😬

alex-page · 2019-06-24T21:06:46Z

This isn't fixed 😭 the issue still exists

chloerice · 2019-07-12T15:41:27Z

Quick update:

I've been trying to narrow down the reason this is an issue, and it seems to only be reproducible with the UserMenu. My changes include:

Handling UserMenu spacing in its parent
- Currently, its handling its own margin-right, which is causing the overlay to be positioned up against the far right since its activator's width was including the margin on its wrapper.
Explicitly setting the UserMenu Popover, rendered by the Menu subcomponent, to have a preferredPosition of right
- Currently, no preferred position was specified, so the calculation for center aligned aligned popover was being returned because the conditional inside of calculateHorizontalPosition wasn't being satisfied.
Calculating and setting right to horizontally position the overlay when preferredAlignment="right" instead calculating and setting left

Unfortunately, while I thought I'd fixed this, in spite of these changes the bug is still present in Firefox and Safari and only fixed in Chrome 😞

… overlay-width

Change made!

github-actions · 2019-10-03T17:25:53Z

Results

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

Files modified	9
Files potentially affected	29

Details

All files potentially affected (total: 29)

❔ .eslintignore (total: 0)

Files potentially affected (total: 0)

❔ 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/utilities/math.ts (total: 29)

Files potentially affected (total: 29)

🎨 src/components/TopBar/TopBar.scss (total: 1)

Files potentially affected (total: 1)

/src/components/TopBar/TopBar.tsx

🎨 src/components/TopBar/components/Menu/Menu.scss (total: 3)

Files potentially affected (total: 3)

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

Files potentially affected (total: 2)

🧩 src/components/TopBar/components/UserMenu/UserMenu.tsx (total: 1)

Files potentially affected (total: 1)

/src/components/TopBar/TopBar.tsx

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.

alex-page · 2019-10-03T18:01:12Z

Replaced with new PR #2231

alex-page self-assigned this Jun 17, 2019

BPScott temporarily deployed to polaris-react-pr-1692 June 17, 2019 17:02 Inactive

alex-page changed the title ~~Fix PositionOverlay width issue for the menu~~ Fix PositionOverlay width issue for the menu in TopBar Jun 17, 2019

alex-page requested review from ken3650 and AndrewMusgrave June 17, 2019 17:18

alex-page added Bug Something is broken and not working as intended in the system. 🌟High Priority labels Jun 17, 2019

BPScott temporarily deployed to polaris-react-pr-1692 June 17, 2019 17:43 Inactive

BPScott temporarily deployed to polaris-react-pr-1692 June 17, 2019 17:45 Inactive

BPScott temporarily deployed to polaris-react-pr-1692 June 18, 2019 13:01 Inactive

BPScott temporarily deployed to polaris-react-pr-1692 June 18, 2019 13:02 Inactive

BPScott temporarily deployed to polaris-react-pr-1692 June 18, 2019 13:27 Inactive

ken3650 approved these changes Jun 18, 2019

View reviewed changes

AndrewMusgrave mentioned this pull request Jun 18, 2019

Cherry-pick change from v4 to master #1700

Merged

BPScott temporarily deployed to polaris-react-pr-1692 June 18, 2019 18:45 Inactive

BPScott temporarily deployed to polaris-react-pr-1692 June 18, 2019 18:47 Inactive

alex-page assigned chloerice and unassigned alex-page Jun 24, 2019

chloerice force-pushed the overlay-width branch from 9346c49 to 192bf3a Compare July 12, 2019 15:44

BPScott temporarily deployed to polaris-react-pr-1692 July 12, 2019 15:44 Inactive

chloerice mentioned this pull request Jul 12, 2019

PositionOverlay incorrectly calculating the left value for right aligned components with scroll bars #1673

Closed

Alex Page and others added 17 commits September 11, 2019 13:49

Add shim for mutation observer

3d97732

Add fixed for coverage

bac15d8

Undo text change

ad6342a

Remove console logs

9882c98

specify preferred position of user menu overlay

215da65

handle spacing of user menu in topbar

17fd0e9

🔥 wrapper with padding around playground

33a947c

calculate the right instead of left for right aligned popovers

f712628

set a consistent scrollbar width

94e41be

fix spacing of popovers positioned above

645d933

update calculateHorizontalPostion to use the consistent scrollbar width

fa092bf

drop (console logs for debugging)

ac9ea90

🔥 scrollbar-width CSS

46e749a

make popover width 100%

bc3415f

fix into scrollbar-width 🔥 commit

f2c5bf6

Remove preferred alignment

208d0b8

consider horizontal margins

d59291f

chloerice force-pushed the overlay-width branch from 208d0b8 to d59291f Compare September 11, 2019 17:54

Alex Page added 4 commits September 11, 2019 13:56

Remove shim test as we use a package now

ba55b03

Merge branch 'overlay-width' of github.com:Shopify/polaris-react into…

5190b78

… overlay-width

Remove eslingignore for mutation observer shim

45a36ca

Remove mutation observer shim

106986a

Alex Page added 3 commits September 11, 2019 14:32

Add 9px gap back

05d6a15

Forgot a }

f6f496e

Update math.ts (#2230)

4eb9db7

alex-page closed this Oct 3, 2019

alex-page deleted the overlay-width branch October 25, 2019 15:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Fix PositionOverlay width issue for the menu in TopBar #1692

[WIP] Fix PositionOverlay width issue for the menu in TopBar #1692

alex-page commented Jun 17, 2019 •

edited by chloerice

Loading

alex-page commented Jun 17, 2019

danrosenthal commented Jun 17, 2019

AndrewMusgrave commented Jun 17, 2019

danrosenthal commented Jun 18, 2019

alex-page commented Jun 18, 2019 •

edited

Loading

danrosenthal commented Jun 18, 2019

ken3650 left a comment

AndrewMusgrave commented Jun 18, 2019

alex-page commented Jun 24, 2019

chloerice commented Jul 12, 2019 •

edited

Loading

github-actions bot commented Oct 3, 2019 •

edited

Loading

Files potentially affected (total: 0)

Files potentially affected (total: 0)

Files potentially affected (total: 25)

Files potentially affected (total: 28)

Files potentially affected (total: 29)

Files potentially affected (total: 1)

Files potentially affected (total: 3)

Files potentially affected (total: 2)

Files potentially affected (total: 1)

alex-page commented Oct 3, 2019 •

edited

Loading

[WIP] Fix PositionOverlay width issue for the menu in TopBar #1692

[WIP] Fix PositionOverlay width issue for the menu in TopBar #1692

Conversation

alex-page commented Jun 17, 2019 • edited by chloerice Loading

WHY are these changes introduced?

WHAT is this pull request doing?

How to 🎩

🎩 checklist

alex-page commented Jun 17, 2019

danrosenthal commented Jun 17, 2019

AndrewMusgrave commented Jun 17, 2019

danrosenthal commented Jun 18, 2019

alex-page commented Jun 18, 2019 • edited Loading

danrosenthal commented Jun 18, 2019

ken3650 left a comment

Choose a reason for hiding this comment

AndrewMusgrave commented Jun 18, 2019

alex-page commented Jun 24, 2019

chloerice commented Jul 12, 2019 • edited Loading

github-actions bot commented Oct 3, 2019 • edited Loading

Results

Details

Files potentially affected (total: 0)

Files potentially affected (total: 0)

Files potentially affected (total: 25)

Files potentially affected (total: 28)

Files potentially affected (total: 29)

Files potentially affected (total: 1)

Files potentially affected (total: 3)

Files potentially affected (total: 2)

Files potentially affected (total: 1)

alex-page commented Oct 3, 2019 • edited Loading

alex-page commented Jun 17, 2019 •

edited by chloerice

Loading

alex-page commented Jun 18, 2019 •

edited

Loading

chloerice commented Jul 12, 2019 •

edited

Loading

github-actions bot commented Oct 3, 2019 •

edited

Loading

alex-page commented Oct 3, 2019 •

edited

Loading