Skip to content

Conversation

AndrewMusgrave
Copy link
Member

@AndrewMusgrave AndrewMusgrave commented May 1, 2019

WHY are these changes introduced?

Part of #1377

WHAT is this pull request doing?

  • Using the new context API for theme provider
  • Updating related utilities
  • Update tests to use the new context
  • Enzyme was running into issues with shallow renders and Consumer / render prop so tests were switch over to mountWithAppProvider

How to 🎩

Copy-paste this code in playground/Playground.tsx:
import * as React from 'react';
import {
  Page,
  AppProvider,
  ThemeProvider,
  TopBar,
  Frame,
  Card,
  ActionList,
  Button,
  ChoiceList,
} from '../src';

interface State {}

enum ThemeChoice {
  AppProviderThemed,
  ThemeProviderThemed,
  AppAndThemeProviderThemed,
  AppProviderThemedSubscription,
  ThemeProviderThemedSubscription,
}

export default class Playground extends React.Component<never, State> {
  state = {
    backgroundOne: '#00848E',
    backgroundTwo: '#BADA55',
    selected: [ThemeChoice.AppProviderThemed],
  };

  handleChange = (selected) => {
    this.setState({selected});
  };

  render() {
    const {backgroundOne, backgroundTwo, selected} = this.state;

    const topBarMarkup = (
      <TopBar
        showNavigationToggle
        userMenu={
          <TopBar.UserMenu
            open={false}
            actions={[
              {
                items: [
                  {content: 'Your profile', icon: 'profile'},
                  {content: 'Log out', icon: 'logOut'},
                ],
              },
              {
                items: [
                  {content: 'Shopify help center'},
                  {content: 'Community forums'},
                ],
              },
            ]}
            name="Ellen Ochoa"
            detail="Ochoa Crafts"
            initials="EO"
            onToggle={() => {}}
          />
        }
        searchField={
          <TopBar.SearchField
            onChange={() => {}}
            value=""
            placeholder="Search"
          />
        }
      />
    );

    const themeApp = {
      colors: {
        topBar: {
          background: '#108043',
        },
      },
      logo: {
        width: 130,
        topBarSource:
          'https://cdn.shopify.com/shopify-marketing_assets/static/shopify-full-color-white.svg',
      },
    };

    const themeTheme = {
      colors: {
        topBar: {
          background: '#BADA55',
        },
      },
      logo: {
        width: 130,
        topBarSource:
          'https://cdn.shopify.com/shopify-marketing_assets/static/shopify-full-color-white.svg',
      },
    };

    const themeSubscriptionOne = {
      colors: {
        topBar: {
          background: backgroundOne,
        },
      },
      logo: {
        width: 130,
        topBarSource:
          'https://cdn.shopify.com/shopify-marketing_assets/static/shopify-full-color-white.svg',
      },
    };

    const themeSubscriptionTwo = {
      colors: {
        topBar: {
          background: backgroundTwo,
        },
      },
      logo: {
        width: 130,
        topBarSource:
          'https://cdn.shopify.com/shopify-marketing_assets/static/shopify-full-color-white.svg',
      },
    };

    const choiceList = (
      <ChoiceList
        title="Choose how to theme"
        choices={[
          {
            label: 'App provider themed',
            value: ThemeChoice.AppProviderThemed,
          },
          {
            label: 'Theme provider themed',
            value: ThemeChoice.ThemeProviderThemed,
          },
          {
            label: 'App provider and theme provider themed',
            value: ThemeChoice.AppAndThemeProviderThemed,
            helpText: 'Theme provider theme overrides app provider theme',
          },
          {
            label: 'App provider themed with subscription',
            value: ThemeChoice.AppProviderThemedSubscription,
            renderChildren: (isSelected) => {
              return (
                isSelected && (
                  <Button
                    onClick={() => {
                      this.setState(({backgroundOne}) => ({
                        backgroundOne:
                          backgroundOne === '#50248F' ? '#00848E' : '#50248F',
                      }));
                    }}
                  >
                    Toggle theme
                  </Button>
                )
              );
            },
          },
          {
            label: 'Theme provider themed with subscription',
            value: ThemeChoice.ThemeProviderThemedSubscription,
            renderChildren: (isSelected) => {
              return (
                isSelected && (
                  <Button
                    onClick={() => {
                      this.setState(({backgroundTwo}) => ({
                        backgroundTwo:
                          backgroundTwo === '#50248F' ? '#BADA55' : '#50248F',
                      }));
                    }}
                  >
                    Toggle theme
                  </Button>
                )
              );
            },
          },
        ]}
        selected={selected}
        onChange={this.handleChange}
      />
    );

    const appProviderThemed = selected[0] === ThemeChoice.AppProviderThemed && (
      <AppProvider theme={themeApp}>
        <Frame topBar={topBarMarkup}>
          <Page title="Playground">{choiceList}</Page>
        </Frame>
      </AppProvider>
    );

    const themeProviderThemed = selected[0] ===
      ThemeChoice.ThemeProviderThemed && (
      <AppProvider>
        <ThemeProvider theme={themeTheme}>
          <Frame topBar={topBarMarkup}>
            <Page title="Playground">{choiceList}</Page>
          </Frame>
        </ThemeProvider>
      </AppProvider>
    );

    const appAndThemeProviderThemed = selected[0] ===
      ThemeChoice.AppAndThemeProviderThemed && (
      <AppProvider theme={themeApp}>
        <ThemeProvider theme={themeTheme}>
          <Frame topBar={topBarMarkup}>
            <Page title="Playground">{choiceList}</Page>
          </Frame>
        </ThemeProvider>
      </AppProvider>
    );

    const appProviderThemedSubscription = selected[0] ===
      ThemeChoice.AppProviderThemedSubscription && (
      <AppProvider theme={themeSubscriptionOne}>
        <Frame topBar={topBarMarkup}>
          <Page title="Playground">{choiceList}</Page>
        </Frame>
      </AppProvider>
    );

    const themeProviderThemedSubscription = selected[0] ===
      ThemeChoice.ThemeProviderThemedSubscription && (
      <AppProvider>
        <ThemeProvider theme={themeSubscriptionTwo}>
          <Frame topBar={topBarMarkup}>
            <Page title="Playground">{choiceList}</Page>
          </Frame>
        </ThemeProvider>
      </AppProvider>
    );

    return (
      <React.Fragment>
        {appProviderThemed}
        {themeProviderThemed}
        {appAndThemeProviderThemed}
        {appProviderThemedSubscription}
        {themeProviderThemedSubscription}
      </React.Fragment>
    );
  }
}

@BPScott BPScott temporarily deployed to polaris-react-pr-1396 May 1, 2019 20:46 Inactive
if (contextOne && 'logo' in contextOne) {
themeContext = contextOne as CreateThemeContext;
appProviderContext = contextTwo;
appProviderContext = contextTwo as CreateAppProviderContext;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CreateThemeContext and CreateAppProviderContext don't have as much overlap anymore

it('is called when a keypress event is registered on the button', () => {
const fakeEventData = {key: 'foo'};
const spy = jest.fn();
shallowWithAppProvider(<Button onKeyPress={spy}>Test</Button>).simulate(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simulate was causing this test and the two below to stall and run indefinitely so I choose to use trigger instead

expect(dateSelector.prop('filterMaxKey')).toBe(filterMaxKey);
});

it('does not render Select with operator options', () => {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DateSelector does render a select 😄

expect(spy).toHaveBeenCalledTimes(1);
});

it('sets focused state to true when the navigation button is focused', () => {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These were purely visual tests, testing the inner state of the component

@AndrewMusgrave AndrewMusgrave force-pushed the themeprovider-new-context branch from 57e8ead to 026f6d3 Compare May 1, 2019 21:00
@BPScott BPScott temporarily deployed to polaris-react-pr-1396 May 1, 2019 21:00 Inactive
@AndrewMusgrave AndrewMusgrave added the 🤖Skip Changelog Causes CI to ignore changelog update check. label May 1, 2019
@@ -0,0 +1,21 @@
import * as React from 'react';
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This never had coverage and was causing codecov to dip below 90

Copy link
Contributor

@dleroux dleroux May 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You think it would be worth adding tests to ensure the polaris prop gets passed to the wrapped component?


describe('withAppProvider', () => {
it('throws when polaris is not defined', () => {
const consoleSpy = jest
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is to suppress the error message being throw

@AndrewMusgrave AndrewMusgrave changed the title [WIP] Use the new context api in ThemeProvider [ThemeProvider] Use the new context api May 1, 2019
@AndrewMusgrave AndrewMusgrave requested a review from dleroux May 1, 2019 22:42
@AndrewMusgrave AndrewMusgrave marked this pull request as ready for review May 1, 2019 22:42
unsubscribe: mockUnsubscribe,
},
});
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like how this is so much more clear 🎉

Copy link
Contributor

@dleroux dleroux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 sorry for the delay on this :shipit:

@AndrewMusgrave AndrewMusgrave merged commit 6a6b6a8 into update-react May 6, 2019
@AndrewMusgrave AndrewMusgrave deleted the themeprovider-new-context branch May 6, 2019 15:11
alex-page pushed a commit that referenced this pull request Jan 30, 2024
…11525)

### WHY are these changes introduced?

Fix for
[#1396](Shopify/polaris-internal#1396).

### WHAT is this pull request doing?

Fixes incorrect border radius styles for `Button` inside the
`ButtonGroup` where the `connectedTop` prop is applied.

Also modifies the logic that originally applied a negative left margin
for all segmented buttons to only apply on `variant="primary"` buttons.
This was causing spacing issues where the width of the ButtonGroup was
1px smaller than the sibling element that it was supposed to match a 0
border radius with.
  <details>
    <summary>ButtonGroup connectedTop — before</summary>
<img
src="https://github.com/Shopify/polaris/assets/26749317/cc8a6f6b-f0c7-4b06-a3d7-3a415e38d5d6"
alt="ButtonGroup connectedTop — before">
  </details>
  <details>
    <summary>ButtonGroup connectedTop — after</summary>
<img
src="https://github.com/Shopify/polaris/assets/26749317/d3b037d3-835f-4ea3-91ef-4ee191a58186"
alt="ButtonGroup connectedTop — after">
  </details>

### How to 🎩

[Spin
URL](https://admin.web.button-group-fix.lo-kim.us.spin.dev/store/shop1/settings/branding)

- Scroll down to "Logo" card section
- Click "Add a default logo"
- Click the high five image and press done

🖥 [Local development
instructions](https://github.com/Shopify/polaris/blob/main/README.md#install-dependencies-and-build-workspaces)
🗒 [General tophatting
guidelines](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md)
📄 [Changelog
guidelines](https://github.com/Shopify/polaris/blob/main/.github/CONTRIBUTING.md#changelog)

### 🎩 checklist

- [x] Tested a
[snapshot](https://github.com/Shopify/polaris/blob/main/documentation/Releasing.md#-snapshot-releases)
- [x] Tested on
[mobile](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md#cross-browser-testing)
- [x] Tested on [multiple
browsers](https://help.shopify.com/en/manual/shopify-admin/supported-browsers)
- [ ] Tested for
[accessibility](https://github.com/Shopify/polaris/blob/main/documentation/Accessibility%20testing.md)
- [ ] Updated the component's `README.md` with documentation changes
- [ ] [Tophatted
documentation](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting%20documentation.md)
changes in the style guide
AnnaCheba pushed a commit to AnnaCheba/polaris that referenced this pull request Apr 22, 2024
…hopify#11525)

### WHY are these changes introduced?

Fix for
[Shopify#1396](Shopify/polaris-internal#1396).

### WHAT is this pull request doing?

Fixes incorrect border radius styles for `Button` inside the
`ButtonGroup` where the `connectedTop` prop is applied.

Also modifies the logic that originally applied a negative left margin
for all segmented buttons to only apply on `variant="primary"` buttons.
This was causing spacing issues where the width of the ButtonGroup was
1px smaller than the sibling element that it was supposed to match a 0
border radius with.
  <details>
    <summary>ButtonGroup connectedTop — before</summary>
<img
src="https://github.com/Shopify/polaris/assets/26749317/cc8a6f6b-f0c7-4b06-a3d7-3a415e38d5d6"
alt="ButtonGroup connectedTop — before">
  </details>
  <details>
    <summary>ButtonGroup connectedTop — after</summary>
<img
src="https://github.com/Shopify/polaris/assets/26749317/d3b037d3-835f-4ea3-91ef-4ee191a58186"
alt="ButtonGroup connectedTop — after">
  </details>

### How to 🎩

[Spin
URL](https://admin.web.button-group-fix.lo-kim.us.spin.dev/store/shop1/settings/branding)

- Scroll down to "Logo" card section
- Click "Add a default logo"
- Click the high five image and press done

🖥 [Local development
instructions](https://github.com/Shopify/polaris/blob/main/README.md#install-dependencies-and-build-workspaces)
🗒 [General tophatting
guidelines](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md)
📄 [Changelog
guidelines](https://github.com/Shopify/polaris/blob/main/.github/CONTRIBUTING.md#changelog)

### 🎩 checklist

- [x] Tested a
[snapshot](https://github.com/Shopify/polaris/blob/main/documentation/Releasing.md#-snapshot-releases)
- [x] Tested on
[mobile](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md#cross-browser-testing)
- [x] Tested on [multiple
browsers](https://help.shopify.com/en/manual/shopify-admin/supported-browsers)
- [ ] Tested for
[accessibility](https://github.com/Shopify/polaris/blob/main/documentation/Accessibility%20testing.md)
- [ ] Updated the component's `README.md` with documentation changes
- [ ] [Tophatted
documentation](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting%20documentation.md)
changes in the style guide
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🤖Skip Changelog Causes CI to ignore changelog update check.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants