Revert #2231 #2377

alex-page · 2019-10-29T17:15:28Z

WHY are these changes introduced?

Fixes #2231 (comment) as I don't have time to rewrite popover and do unit tests.

WHAT is this pull request doing?

Reverts #2231

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-29T17:17:54Z

💦 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)

📄 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.

dleroux · 2019-10-30T12:55:30Z

I just remembered @alex-page @loic-d needed this change because of a different bug. Reverting it will bring that bug back. I wonder if reverting is the best approach. Loic, did you have a workaround? Is your change with the broken popover in production? I'm not sure if I can paste the screen here so let me know if you need more context.

dleroux · 2019-10-30T13:00:31Z

I can confirm this revert does work but brings back the old issues of course.

dleroux

I'll approve since the bug is probably more visible.

I've tophatted in the admin.

loic-d · 2019-10-30T13:34:10Z

@dleroux No workaround for us at the moment. I believe it should be addressed in Polaris.
#2231 (comment) seems to be more critical than our issue, so reverting the PR makes sense 👍
Will you work on a fix soon?

dleroux · 2019-10-30T13:48:38Z

We have attempted a few times time to fix it now. @alex-page do you plan on continuing on this?

alex-page · 2019-10-30T14:05:19Z

@dleroux I'm not sure if I have time, I have an open PR up to continue the work in #2362

Revert 2231

3abc0ec

alex-page requested review from dleroux and tmlayton October 29, 2019 17:15

alex-page self-assigned this Oct 29, 2019

Add pr link

b9191f0

dleroux approved these changes Oct 30, 2019

View reviewed changes

Alex Page added 2 commits October 30, 2019 10:05

Merge branch 'master' into revert-2231

68c327d

Merge branch 'master' into revert-2231

9685e6e

alex-page merged commit 47d0240 into master Oct 31, 2019

alex-page deleted the revert-2231 branch October 31, 2019 13:21

jtrollia mentioned this pull request Oct 31, 2019

[ChoiceList] Add JSX support to the title prop #2355

Merged

6 tasks

BPScott mentioned this pull request Oct 31, 2019

Move global css into AppProvider #2392

Merged

LauraAubin mentioned this pull request Feb 25, 2020

[Tabs] Apply design system #2744

Merged

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revert #2231 #2377

Revert #2231 #2377

alex-page commented Oct 29, 2019 •

edited

Loading

github-actions bot commented Oct 29, 2019 •

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

Files potentially affected (total: 3)

Files potentially affected (total: 2)

Files potentially affected (total: 1)

dleroux commented Oct 30, 2019 •

edited

Loading

dleroux commented Oct 30, 2019

dleroux left a comment

loic-d commented Oct 30, 2019 •

edited by dleroux

Loading

dleroux commented Oct 30, 2019

alex-page commented Oct 30, 2019

Revert #2231 #2377

Revert #2231 #2377

Conversation

alex-page commented Oct 29, 2019 • edited Loading

WHY are these changes introduced?

WHAT is this pull request doing?

How to 🎩

🎩 checklist

github-actions bot commented Oct 29, 2019 • 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: 1)

Files potentially affected (total: 3)

Files potentially affected (total: 2)

Files potentially affected (total: 1)

dleroux commented Oct 30, 2019 • edited Loading

dleroux commented Oct 30, 2019

dleroux left a comment

Choose a reason for hiding this comment

loic-d commented Oct 30, 2019 • edited by dleroux Loading

dleroux commented Oct 30, 2019

alex-page commented Oct 30, 2019

alex-page commented Oct 29, 2019 •

edited

Loading

github-actions bot commented Oct 29, 2019 •

edited

Loading

dleroux commented Oct 30, 2019 •

edited

Loading

loic-d commented Oct 30, 2019 •

edited by dleroux

Loading