Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[IndexFilters] consolidate se23 styles and logic #10230

Merged
merged 4 commits into from
Aug 28, 2023
Merged

[IndexFilters] consolidate se23 styles and logic #10230

merged 4 commits into from
Aug 28, 2023

Conversation

gwyneplaine
Copy link
Contributor

@gwyneplaine gwyneplaine commented Aug 25, 2023

WHY are these changes introduced?

Fixes #9938

WHAT is this pull request doing?

  • Consolidate se23 styles
  • Consolidate se23 logic
  • Remove custom FilterButton and replace it with direct invocations of the Polaris Button instead. This seems safe to do because:
    • even though we export FilterButton from the root of IndexFilters, we do not actually expose it directly from the polaris-react entrypoint.
    • AFAIK, noone is reaching into our cjs/esm builds to grabFilterButton (see grokt)
    • Just to be sure, also doesn't look like anyone is importing FilterButton from @shopify/polaris either. (see grokt)
    • That said I can see an argument being made for providing convenience wrappers to alleviate repetitive config, so deferring to the team. 👍 Happy to revert if necessary 👍

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

Copy-paste this code in playground/Playground.tsx:
import React from 'react';
import {Page} from '../src';

export function Playground() {
  return (
    <Page title="Playground">
      {/* Add the code you want to test in here */}
    </Page>
  );
}

🎩 checklist

Copy link
Contributor

@laurkim laurkim left a comment

Choose a reason for hiding this comment

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

Nice 🚀

Small issue with spacing of filter and sort buttons on xs and sm breakpoints but other than that, looks great 🙌
abc25-14-nhd1z-kug3z

Copy link
Contributor

@sophschneider sophschneider left a comment

Choose a reason for hiding this comment

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

Nice! removing the custom button is good w me :) Can you add a changeset for it?

@gwyneplaine
Copy link
Contributor Author

Nice 🚀

Small issue with spacing of filter and sort buttons on xs and sm breakpoints but other than that, looks great 🙌 abc25-14-nhd1z-kug3z

Looks to be an inadvertent bugfix. New spacing is true to Figma
Screenshot 2023-08-28 at 2 12 26 pm

Production:
Screenshot 2023-08-28 at 2 14 16 pm

}
}

.SmallScreenTabsWrapper {
overflow: hidden;
padding: var(--p-space-1) var(--p-space-0) var(--p-space-2) var(--p-space-3);
padding: 0;
height: auto;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was overriding the .TabsWrapper height invocation on mdDown above (line 36).
As this class is only applied at mdDown, safe for us to remove this duplicative height rule.

@gwyneplaine gwyneplaine merged commit a573e55 into next Aug 28, 2023
16 checks passed
@gwyneplaine gwyneplaine deleted the v12/9938 branch August 28, 2023 04:47
This was referenced Aug 28, 2023
sophschneider pushed a commit that referenced this pull request Sep 19, 2023
<!--
  ☝️How to write a good PR title:
- Prefix it with [ComponentName] (if applicable), for example: [Button]
  - Start with a verb, for example: Add, Delete, Improve, Fix…
  - Give as much context as necessary and as little as possible
  - Prefix it with [WIP] while it’s a work in progress
-->

Fixes #9938

* Consolidate se23 styles
* Consolidate se23 logic
* Remove custom `FilterButton` and replace it with direct invocations of
the Polaris Button instead. This seems safe to do because:
* even though we export `FilterButton` from the root of `IndexFilters`,
we do not actually expose it directly from the `polaris-react`
[entrypoint](https://github.com/Shopify/polaris/blob/2c718dc3ecf9393119cd1e08c387b648db78449a/polaris-react/src/index.ts#L190).
* AFAIK, noone is reaching into our cjs/esm builds to grab`FilterButton`
(see
[grokt](https://grokt.shopify.io/results?q=%40shopify%2Fpolaris%2F.*%2Fcomponents%2FIndexFilters))
* Just to be sure, also doesn't look like anyone is importing
`FilterButton` from @shopify/polaris either. (see
[grokt](https://grokt.shopify.io/results?q=FilterButton))
* That said I can see an argument being made for providing convenience
wrappers to alleviate repetitive config, so deferring to the team. 👍
Happy to revert if necessary 👍

🖥 [Local development
instructions](https://github.com/Shopify/polaris/blob/main/README.md#local-development)
🗒 [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)

<!--
  Give as much information as needed to experiment with the component
  in the playground.
-->

<details>
<summary>Copy-paste this code in
<code>playground/Playground.tsx</code>:</summary>

```jsx
import React from 'react';
import {Page} from '../src';

export function Playground() {
  return (
    <Page title="Playground">
      {/* Add the code you want to test in here */}
    </Page>
  );
}
```

</details>

- [ ] Tested on
[mobile](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md#cross-browser-testing)
- [ ] 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
sam-b-rose pushed a commit that referenced this pull request Sep 27, 2023
This PR was opened by the [Changesets
release](https://github.com/changesets/action) GitHub action. When
you're ready to do a release, you can merge this and the packages will
be published to npm automatically. If you're not ready to do a release
yet, that's fine, whenever you add more changesets to next, this PR will
be updated.

⚠️⚠️⚠️⚠️⚠️⚠️

`next` is currently in **pre mode** so this branch has prereleases
rather than normal releases. If you want to exit prereleases, run
`changeset pre exit` on `next`.

⚠️⚠️⚠️⚠️⚠️⚠️

# Releases
## @shopify/polaris@12.0.0-beta.1

### Major Changes

- [#10122](#10122)
[`43b42aefed`](43b42ae)
Thanks [@aveline](https://github.com/aveline)! - Removed `shape` prop on
`Avatar` component


- [#10270](#10270)
[`b9bcaef41`](b9bcaef)
Thanks [@kyledurand](https://github.com/kyledurand)! - Changed `spacing`
prop to `gap` on `List` and `DescriptionList`


- [#9997](#9997)
[`b59fc9e27`](b59fc9e)
Thanks [@yurm04](https://github.com/yurm04)! - [ButtonGroup] Removed
`segmented` boolean prop and replaced with `variant`. Replaced `spacing`
prop with `gap`


- [#10100](#10100)
[`4c7b2d4858`](4c7b2d4)
Thanks [@kyledurand](https://github.com/kyledurand)! - Updated
`borderRadius` props to match web spec


- [#10051](#10051)
[`69edd97ceb`](69edd97)
Thanks [@aveline](https://github.com/aveline)! - Renamed `color` prop to
`tone` for `ProgressBar` component


- [#10182](#10182)
[`e814c0ee1a`](e814c0e)
Thanks [@kyledurand](https://github.com/kyledurand)! - Removed
connectedDislosure prop on button


- [#10283](#10283)
[`42ee9f407`](42ee9f4)
Thanks [@sam-b-rose](https://github.com/sam-b-rose)! - Renamed `size`
prop values for the Avatar component. See the following table for the
new prop mappings.

    | Before                    | After       |
    | ------------------------- | ----------- |
    | `size="extraSmall"`       | `size="xs"` |
    | `size="small"`            | `size="sm"` |
    | `size="medium"`           | `size="md"` |
    | `size="large"`            | `size="lg"` |
    | `size="xl-experimental"`  | `size="xl"` |
    | `size="2xl-experimental"` | `size="xl"` |


- [#10232](#10232)
[`eb2c2035c`](eb2c203)
Thanks [@laurkim](https://github.com/laurkim)! - Removed `divider` prop
from `Page` component


- [#10271](#10271)
[`1125087b5`](1125087)
Thanks [@kyledurand](https://github.com/kyledurand)! - Removed
deprecated additionalNavigation prop on Page Header


- [#10164](#10164)
[`af9f264b9a`](af9f264)
Thanks [@aveline](https://github.com/aveline)! - Renamed `destructive`
prop to `tone` for `Button` component


- [#10261](#10261)
[`abeef7ad0`](abeef7a)
Thanks [@kyledurand](https://github.com/kyledurand)! - Replaced `small`,
`large`, and `fullScreen` props with `size` prop


- [#10206](#10206)
[`dad09bde9`](dad09bd)
Thanks [@kyledurand](https://github.com/kyledurand)! - Changed `status`
prop on `Banner` to `tone`


- [#10220](#10220)
[`2b0cdb2fbf`](2b0cdb2)
Thanks [@kyledurand](https://github.com/kyledurand)! - Changed `color`
prop on `Icon` to `tone`


- [#10036](#10036)
[`359614cf83`](359614c)
Thanks [@kyledurand](https://github.com/kyledurand)! - Replaced
`borderless` prop with `variant` on TextField

Migration: `npx @shopify/polaris-migrator react-rename-component-prop
<path> --componentName="TextField" --from="borderless" --to="variant"
--newValue="borderless"`


- [#10635](#10635)
[`340e36e7d`](340e36e)
Thanks [@laurkim](https://github.com/laurkim)! - Removed
`polarisSummerEditions2023` feature flag from AppProvider context


- [#10090](#10090)
[`4caed28a77`](4caed28)
Thanks [@aveline](https://github.com/aveline)! - Consolidated boolean
`Button` props into `variant` prop


- [#10041](#10041)
[`8f927f7622`](8f927f7)
Thanks [@kyledurand](https://github.com/kyledurand)! - Replaced boolean
props: `secondary`, `fullWidth`, `oneHalf`, `oneThird` on Layout.Section
with `variant`


- [#10266](#10266)
[`22a51eae2`](22a51ea)
Thanks [@kyledurand](https://github.com/kyledurand)! - Renamed `color`
prop on Text to `tone`


- [#9993](#9993)
[`66f5c8df3e`](66f5c8d)
Thanks [@gwyneplaine](https://github.com/gwyneplaine)! - Removed Summer
Editions experimental styles and code for the following components:
`AppProvider`, `Avatar`, `AccountConnection`, `ActionList`,
`ActionMenu`, `Autocomplete`, `Badge`, `Banner`, `Breadcrumbs`,
`BulkActions`, `Button`, `ButtonGroup`, `CalloutCard`, `Card`,
`CheckableButton`, `Checkbox`, `Choice`, `Connected`, `DataTable`,
`DatePicker`, `DropZone`, `EmptyState`, `Filters`, `FormLayout`,
`Frame`, `FullscreenBar`, `IndexFilters`, `IndexTable`, `InlineError`,
`KeyboardKey`, `Labelled`, `Layout`, `LegacyCard`, `LegacyFilters`,
`LegacyTabs`, `Link`, `List`, `Listbox`, `MediaCard`, `Modal`,
`Navigation`, `OptionList`, `Page`, `PageActions`, `Pagination`,
`Popover`, `ProgressBar`, `RadioButton`, `ResourceItem`, `ResourceList`,
`Select`, `SettingAction`, `ShadowBevel`, `SkeletonPage`,
`SkeletonThumbnail`, `Spinner`, `Tabs`, `Tag`, `Text`, `TextField`,
`Thumbnail`, `TooltipOverlay`, `TopBar`, and `VideoThumbnail`


- [#10232](#10232)
[`eb2c2035c`](eb2c203)
Thanks [@laurkim](https://github.com/laurkim)! - Removed `optionRole`
prop from `OptionList` component

### Minor Changes

- [#10238](#10238)
[`b17d23d69`](b17d23d)
Thanks [@sam-b-rose](https://github.com/sam-b-rose)! - Add a search
field to filter ActionList that have more than 10 items

### Patch Changes

- [#10228](#10228)
[`e18ca907e`](e18ca90)
Thanks [@gwyneplaine](https://github.com/gwyneplaine)! - `Filters`
Remove unused disabled states in `FilterPill`


- [#10268](#10268)
[`dbe3d9ece`](dbe3d9e)
Thanks [@laurkim](https://github.com/laurkim)! - Fixed broken focus ring
styles on `ResourceItem` component


- [#10238](#10238)
[`b17d23d69`](b17d23d)
Thanks [@sam-b-rose](https://github.com/sam-b-rose)! - Ensure Avatar has
no background color if an source prop is passed in to allow for
transparent images


- [#10230](#10230)
[`a573e55d0`](a573e55)
Thanks [@gwyneplaine](https://github.com/gwyneplaine)! - `IndexFilter`
remove custom `FilterButton` in favor of directly invoking the Polaris
`Button` component.

- Updated dependencies
\[[`86d4040c0`](86d4040)]:
    -   @shopify/polaris-tokens@7.13.0-beta.0

## @shopify/polaris-tokens@7.13.0-beta.0

### Minor Changes

- [#10382](#10382)
[`86d4040c0`](86d4040)
Thanks [@laurkim](https://github.com/laurkim)! - Renamed `ThemeVariant`
to `Theme` and exposed `Theme` type

## @shopify/stylelint-polaris@14.1.2-beta.0

### Patch Changes

- Updated dependencies
\[[`86d4040c0`](86d4040)]:
    -   @shopify/polaris-tokens@7.13.0-beta.0

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
AnnaCheba pushed a commit to AnnaCheba/polaris that referenced this pull request Apr 22, 2024
<!--
  ☝️How to write a good PR title:
- Prefix it with [ComponentName] (if applicable), for example: [Button]
  - Start with a verb, for example: Add, Delete, Improve, Fix…
  - Give as much context as necessary and as little as possible
  - Prefix it with [WIP] while it’s a work in progress
-->

### WHY are these changes introduced?

Fixes Shopify#9938 

### WHAT is this pull request doing?

* Consolidate se23 styles 
* Consolidate se23 logic
* Remove custom `FilterButton` and replace it with direct invocations of
the Polaris Button instead. This seems safe to do because:
* even though we export `FilterButton` from the root of `IndexFilters`,
we do not actually expose it directly from the `polaris-react`
[entrypoint](https://github.com/Shopify/polaris/blob/0151ec04e4a72a657fc4f3ca53e8990c58a47cfa/polaris-react/src/index.ts#L190).
* AFAIK, noone is reaching into our cjs/esm builds to grab`FilterButton`
(see
[grokt](https://grokt.shopify.io/results?q=%40shopify%2Fpolaris%2F.*%2Fcomponents%2FIndexFilters))
* Just to be sure, also doesn't look like anyone is importing
`FilterButton` from @shopify/polaris either. (see
[grokt](https://grokt.shopify.io/results?q=FilterButton))
* That said I can see an argument being made for providing convenience
wrappers to alleviate repetitive config, so deferring to the team. 👍
Happy to revert if necessary 👍

### How to 🎩

🖥 [Local development
instructions](https://github.com/Shopify/polaris/blob/main/README.md#local-development)
🗒 [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)

<!--
  Give as much information as needed to experiment with the component
  in the playground.
-->

<details>
<summary>Copy-paste this code in
<code>playground/Playground.tsx</code>:</summary>

```jsx
import React from 'react';
import {Page} from '../src';

export function Playground() {
  return (
    <Page title="Playground">
      {/* Add the code you want to test in here */}
    </Page>
  );
}
```

</details>

### 🎩 checklist

- [ ] Tested on
[mobile](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md#cross-browser-testing)
- [ ] 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
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[IndexFilters] Consolidate se23 logic and styles
4 participants