[WIP] Fix calculation for center aligned popover #2362

alex-page · 2019-10-25T15:09:01Z

WHY are these changes introduced?

Re-re- fixes #1673
Fixes #2231 (comment)

WHAT is this pull request doing?

Remove horizontal margin on popover overlay.
Right aligned popovers positioned with left value
Simplify and update math.ts
Remove prefferedAlignment="right" on topbar

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

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
For visual design changes, pinged one of @ HYPD, @ mirualves, @ sarahill, or @ ry5n to update the Polaris UI kit

github-actions · 2019-10-25T15:11:29Z

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

Files modified	5
Files potentially affected	29

Details

All files potentially affected (total: 29)

📄 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/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-25T15:33:37Z

Tophatting this fix in web locally. It currently looks like this, if we want it aligned with the activator on the right we need to set prefferedAlignment="right" for that component in web:

I can't replicate the issue @AndrewMusgrave had here #2231 (comment)

alex-page · 2019-10-25T15:44:29Z

src/components/Popover/Popover.scss

-  margin: $visible-portion-of-arrow spacing(tight) spacing();
+  margin: $visible-portion-of-arrow 0 0;


I'm not sure why the popover needed margin on the horizontal axis as it is positioned absolutely.

alex-page · 2019-10-25T15:46:08Z

src/components/PositionedOverlay/PositionedOverlay.tsx

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


Remove the right value and only positioning the popover with the left value. This is how it previously worked.

This simplifies the math when the popover overlaps the page. It now doesn't have to calculate a three right values it can just switch to a previously defined left value.

alex-page · 2019-10-25T15:48:01Z

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

@@ -55,7 +55,6 @@ export function UserMenu({

  return (
    <Menu
-      preferredAlignment="right"


This should work with default middle, left or right alignment.

If the popover overlaps the right hand side of the page it should use a different left value.

tmlayton · 2019-10-25T23:01:18Z

Can we add a regression test for the previous bug? Also, do we have good test cases for the different positions? I’m just weary of this much code changing without any tests changing/being added that, while we might fix #2231 (comment), we could inadvertently introduce a new positioning bug.

alex-page · 2020-01-13T18:47:53Z

Closing as #2587 shipped. Thanks Tim.

Remove horizontal margin, right align use left, update math.ts

5964db4

alex-page requested review from chloerice and tmlayton October 25, 2019 15:09

alex-page self-assigned this Oct 25, 2019

alex-page requested a review from AndrewMusgrave October 25, 2019 15:12

Alex Page added 2 commits October 25, 2019 11:38

Fix formatting

c154e63

Add changelog

53b036b

alex-page commented Oct 25, 2019

View reviewed changes

alex-page changed the title ~~Fix calculation for center aligned popover~~ [WIP] Fix calculation for center aligned popover Oct 25, 2019

alex-page mentioned this pull request Oct 30, 2019

Revert #2231 #2377

Merged

6 tasks

AndrewMusgrave removed their request for review November 4, 2019 22:11

alex-page assigned kyledurand Dec 9, 2019

alex-page mentioned this pull request Jan 10, 2020

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

Merged

6 tasks

alex-page closed this Jan 13, 2020

alex-page deleted the popover-pagescroll branch January 13, 2020 18:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Fix calculation for center aligned popover #2362

[WIP] Fix calculation for center aligned popover #2362

alex-page commented Oct 25, 2019 •

edited by chloerice

Loading

github-actions bot commented Oct 25, 2019 •

edited

Loading

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)

alex-page commented Oct 25, 2019 •

edited

Loading

alex-page Oct 25, 2019

alex-page Oct 25, 2019 •

edited

Loading

alex-page Oct 25, 2019

tmlayton commented Oct 25, 2019 •

edited

Loading

alex-page commented Jan 13, 2020

		margin: $visible-portion-of-arrow spacing(tight) spacing();
		margin: $visible-portion-of-arrow 0 0;

[WIP] Fix calculation for center aligned popover #2362

[WIP] Fix calculation for center aligned popover #2362

Conversation

alex-page commented Oct 25, 2019 • edited by chloerice Loading

WHY are these changes introduced?

WHAT is this pull request doing?

How to 🎩

🎩 checklist

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

Details

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)

alex-page commented Oct 25, 2019 • edited Loading

alex-page Oct 25, 2019

Choose a reason for hiding this comment

alex-page Oct 25, 2019 • edited Loading

Choose a reason for hiding this comment

alex-page Oct 25, 2019

Choose a reason for hiding this comment

tmlayton commented Oct 25, 2019 • edited Loading

alex-page commented Jan 13, 2020

alex-page commented Oct 25, 2019 •

edited by chloerice

Loading

github-actions bot commented Oct 25, 2019 •

edited

Loading

alex-page commented Oct 25, 2019 •

edited

Loading

alex-page Oct 25, 2019 •

edited

Loading

tmlayton commented Oct 25, 2019 •

edited

Loading