[Popover][TopBar] Overlay width fix #2231

alex-page · 2019-10-03T17:29:40Z

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

github-actions · 2019-10-03T18:02:42Z

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

Files modified	10
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/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/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.

src/components/Popover/Popover.scss

src/components/PositionedOverlay/utilities/math.ts

src/components/TopBar/components/Menu/Menu.scss

src/components/TopBar/components/UserMenu/UserMenu.tsx

chloerice

Tophat in web looks solid 🙌

Aside from the UserMenu, I tophatted popovers on home cards, Discounts timeline, and Orders and Collections resource lists. Tophatted modals (to ensure there were no regressions from PositionedOverlay changes) in Discounts and Products.

Before	After

dleroux

Nice work!

src/components/PositionedOverlay/utilities/math.ts

loic-d · 2019-10-16T19:22:52Z

@alex-page This bug was affecting us in web with Tooltip.

Your fix seems to work 🎩 👍

tmlayton · 2019-10-17T05:07:45Z

src/components/PositionedOverlay/PositionedOverlay.tsx

@@ -118,20 +119,30 @@ export class PositionedOverlay extends React.PureComponent<
  }

  render() {
-    const {left, top, zIndex, width} = this.state;
-    const {render, fixed, classNames: propClassNames} = this.props;


Removing propClassNames and not passing to classNames below broke popover CSS transitions. I opened an issue for it #2310

Before After

tmlayton · 2019-10-24T01:57:47Z

I just shipped v4.7.0 and v4.7.1 updates to web and noticed this where popover is rendering off canvas and causing horizontal scrolling in some cases:

I’m not sure if this was a preexisting condition. @alex-page can you verify?

AndrewMusgrave · 2019-10-24T22:51:40Z

I can re-create this as well, it definitely wasn't occurring previously. @alex-page was looking at this earlier today and it didn't appear off screen (I'm on chrome 77)

alex-page · 2019-10-25T13:32:56Z

This is caused by this PR. There is a bug with middle aligned popover on the right side of the page. I am looking into this now.

* Revert 2231

alex-page assigned chloerice and alex-page Oct 3, 2019

alex-page mentioned this pull request Oct 3, 2019

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

Closed

5 tasks

alex-page changed the title ~~Overlay width fix~~ [Popover][TopBar] Overlay width fix Oct 9, 2019

alex-page requested review from AndrewMusgrave and dleroux October 10, 2019 15:36

alex-page commented Oct 10, 2019

View reviewed changes

src/components/Popover/Popover.scss Show resolved Hide resolved

alex-page commented Oct 10, 2019

View reviewed changes

src/components/PositionedOverlay/utilities/math.ts Show resolved Hide resolved

alex-page commented Oct 10, 2019

View reviewed changes

src/components/TopBar/components/Menu/Menu.scss Show resolved Hide resolved

alex-page commented Oct 10, 2019

View reviewed changes

src/components/TopBar/components/UserMenu/UserMenu.tsx Show resolved Hide resolved

chloerice force-pushed the overlay-width-fix branch 2 times, most recently from b776dfc to 297c43d Compare October 11, 2019 19:28

chloerice approved these changes Oct 11, 2019

View reviewed changes

dleroux approved these changes Oct 15, 2019

View reviewed changes

dleroux reviewed Oct 16, 2019

View reviewed changes

src/components/PositionedOverlay/utilities/math.ts Show resolved Hide resolved

Fixed position overlay positioning

fe8819d

dleroux force-pushed the overlay-width-fix branch from 297c43d to fe8819d Compare October 16, 2019 19:45

alex-page merged commit 20a72e3 into master Oct 16, 2019

alex-page deleted the overlay-width-fix branch October 16, 2019 19:56

tmlayton reviewed Oct 17, 2019

View reviewed changes

tmlayton mentioned this pull request Oct 17, 2019

[Popover] Bring back pass through classes #2311

Merged

1 task

This was referenced Oct 25, 2019

[WIP] Fix calculation for center aligned popover #2362

Closed

Revert #2231 #2377

Merged

alex-page pushed a commit that referenced this pull request Oct 31, 2019

Revert #2231 (#2377)

47d0240

* Revert 2231

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Popover][TopBar] Overlay width fix #2231

[Popover][TopBar] Overlay width fix #2231

alex-page commented Oct 3, 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: 0)

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)

chloerice left a comment

dleroux left a comment

loic-d commented Oct 16, 2019

tmlayton Oct 17, 2019 •

edited

Loading

tmlayton commented Oct 24, 2019 •

edited

Loading

AndrewMusgrave commented Oct 24, 2019

alex-page commented Oct 25, 2019

[Popover][TopBar] Overlay width fix #2231

[Popover][TopBar] Overlay width fix #2231

Conversation

alex-page commented Oct 3, 2019 • edited Loading

WHY are these changes introduced?

WHAT is this pull request doing?

How to 🎩

🎩 checklist

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

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: 0)

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)

chloerice left a comment

Choose a reason for hiding this comment

dleroux left a comment

Choose a reason for hiding this comment

loic-d commented Oct 16, 2019

tmlayton Oct 17, 2019 • edited Loading

Choose a reason for hiding this comment

tmlayton commented Oct 24, 2019 • edited Loading

AndrewMusgrave commented Oct 24, 2019

alex-page commented Oct 25, 2019

alex-page commented Oct 3, 2019 •

edited

Loading

github-actions bot commented Oct 3, 2019 •

edited

Loading

tmlayton Oct 17, 2019 •

edited

Loading

tmlayton commented Oct 24, 2019 •

edited

Loading