-
Notifications
You must be signed in to change notification settings - Fork 1.2k
[Storybook] Fix font-loading in Chromatic #9993
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
Conversation
### WHY are these changes introduced? Related to #9899 <!-- Context about the problem that’s being addressed. --> ### WHAT is this pull request doing? Removes beta flag logic and merge override styles for Avatar component.
|
see https://shopify.chromatic.com/build?appId=5d559397bae39100201eedc1&number=19008 and note that the baseline is rendering off-kilter. 🧵 Slack Thread
Created originally at 2023-08-10 07:03AM UTC by |
8f0067d to
08fd9eb
Compare
Co-authored-by: Jess Telford <jess+github@jes.st>
|
I'm still getting the layout shift/font flash on storybook when I pulled this branch, Edit: my solution doesn't work either it just loaded the system font again :( |
| fonts: await document.fonts.ready, | ||
| }); | ||
|
|
||
| // Exporting as a loader from `preview.js` forces the promise to be resolved before EVERY story |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I debuggered in some components and they were rendering pre inter loading 🤔 I wonder if we have to check if inter is ready specifically
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be because of the isChromatic check... I guess it makes sense to force loading Inter on local dev too, so we could remove that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried removing it but it still flashes the SF font locally...
is there any way we can test that it isn't flashing on chromatic?
It might be worth merging both this approach AND #10073?
5d459f8 to
700a72f
Compare
polaris-react/.storybook/preview.js
Outdated
| import isChromatic from 'chromatic/isChromatic'; | ||
|
|
||
| // Use the document.fonts API to ensure fonts have fully loaded before rendering a story. Important for consistency when doing visual diffing in Chromatic. | ||
| // See: https://developer.mozilla.org/en-US/docs/Web/API/Document/fonts | ||
| const fontLoader = async () => ({ | ||
| fonts: await document.fonts.ready, | ||
| }); | ||
|
|
||
| // Exporting as a loader from `preview.js` forces the promise to be resolved before EVERY story | ||
| // See: https://storybook.js.org/docs/react/writing-stories/loaders | ||
| export const loaders = isChromatic() && document.fonts ? [fontLoader] : []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| import isChromatic from 'chromatic/isChromatic'; | |
| // Use the document.fonts API to ensure fonts have fully loaded before rendering a story. Important for consistency when doing visual diffing in Chromatic. | |
| // See: https://developer.mozilla.org/en-US/docs/Web/API/Document/fonts | |
| const fontLoader = async () => ({ | |
| fonts: await document.fonts.ready, | |
| }); | |
| // Exporting as a loader from `preview.js` forces the promise to be resolved before EVERY story | |
| // See: https://storybook.js.org/docs/react/writing-stories/loaders | |
| export const loaders = isChromatic() && document.fonts ? [fontLoader] : []; | |
| // Use the document.fonts API to ensure fonts have fully loaded before rendering a story. Important for consistency when doing visual diffing in Chromatic. | |
| // See: https://developer.mozilla.org/en-US/docs/Web/API/Document/fonts | |
| const fontLoader = async () => ({ | |
| fonts: await document.fonts.ready, | |
| }); | |
| // Exporting as a loader from `preview.js` forces the promise to be resolved before EVERY story | |
| // See: https://storybook.js.org/docs/react/writing-stories/loaders | |
| export const loaders = document.fonts ? [fontLoader] : []; |
Also force fonts to load on local dev, not just Chromatic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure how to test how this is working on chromatic but lets just merge it in and see what happens!
My PR #10073 gets the loading working locally and on chromatic, but as @jesstelford mentioned the google font urls could change.
I honestly think its safe for us to merge both A and B approaches from here
Edit: I tested solution A and solution B and it seems like both methods are not working in the chromatic snapshots 🤔 I agree that solution B is the best approach if we can get it to work.
I'm going to close the solution A pr for now, but maybe we should consider solution C (using system font for chromatic) if we can't get this to work
|
Merging this work around solution to use the system font instead until we can verify that this PR will fix the flakiness. I still like this PR's approach the best! If we can get it to work, we can revert the changes in the work around solution |
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>
|
Since this is a small PR and it's way behind in git history, I'm gonna close this in favour of mirroring the changes on a new branch to avoid git history murkiness 👍 cc @jesstelford @sophschneider |
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 - [Shopify#10122](Shopify#10122) [`60d4139679`](Shopify@60d4139) Thanks [@aveline](https://github.com/aveline)! - Removed `shape` prop on `Avatar` component - [Shopify#10270](Shopify#10270) [`b9bcaef41`](Shopify@8bf1fe7) Thanks [@kyledurand](https://github.com/kyledurand)! - Changed `spacing` prop to `gap` on `List` and `DescriptionList` - [Shopify#9997](Shopify#9997) [`b59fc9e27`](Shopify@17f55da) Thanks [@yurm04](https://github.com/yurm04)! - [ButtonGroup] Removed `segmented` boolean prop and replaced with `variant`. Replaced `spacing` prop with `gap` - [Shopify#10100](Shopify#10100) [`b367e86e79`](Shopify@b367e86) Thanks [@kyledurand](https://github.com/kyledurand)! - Updated `borderRadius` props to match web spec - [Shopify#10051](Shopify#10051) [`537dabfe51`](Shopify@537dabf) Thanks [@aveline](https://github.com/aveline)! - Renamed `color` prop to `tone` for `ProgressBar` component - [Shopify#10182](Shopify#10182) [`5f970801ad`](Shopify@5f97080) Thanks [@kyledurand](https://github.com/kyledurand)! - Removed connectedDislosure prop on button - [Shopify#10283](Shopify#10283) [`42ee9f407`](Shopify@15352ad) 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"` | - [Shopify#10232](Shopify#10232) [`eb2c2035c`](Shopify@799ce67) Thanks [@laurkim](https://github.com/laurkim)! - Removed `divider` prop from `Page` component - [Shopify#10271](Shopify#10271) [`1125087b5`](Shopify@4c07562) Thanks [@kyledurand](https://github.com/kyledurand)! - Removed deprecated additionalNavigation prop on Page Header - [Shopify#10164](Shopify#10164) [`ba1fb8d75e`](Shopify@ba1fb8d) Thanks [@aveline](https://github.com/aveline)! - Renamed `destructive` prop to `tone` for `Button` component - [Shopify#10261](Shopify#10261) [`abeef7ad0`](Shopify@1b7e8e2) Thanks [@kyledurand](https://github.com/kyledurand)! - Replaced `small`, `large`, and `fullScreen` props with `size` prop - [Shopify#10206](Shopify#10206) [`dad09bde9`](Shopify@a1d9087) Thanks [@kyledurand](https://github.com/kyledurand)! - Changed `status` prop on `Banner` to `tone` - [Shopify#10220](Shopify#10220) [`a2af4ae619`](Shopify@a2af4ae) Thanks [@kyledurand](https://github.com/kyledurand)! - Changed `color` prop on `Icon` to `tone` - [Shopify#10036](Shopify#10036) [`68d6223b4d`](Shopify@68d6223) 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"` - [Shopify#10635](Shopify#10635) [`340e36e7d`](Shopify@d2288d8) Thanks [@laurkim](https://github.com/laurkim)! - Removed `polarisSummerEditions2023` feature flag from AppProvider context - [Shopify#10090](Shopify#10090) [`cf80608dae`](Shopify@cf80608) Thanks [@aveline](https://github.com/aveline)! - Consolidated boolean `Button` props into `variant` prop - [Shopify#10041](Shopify#10041) [`ce6f75b5a3`](Shopify@ce6f75b) Thanks [@kyledurand](https://github.com/kyledurand)! - Replaced boolean props: `secondary`, `fullWidth`, `oneHalf`, `oneThird` on Layout.Section with `variant` - [Shopify#10266](Shopify#10266) [`22a51eae2`](Shopify@5d671ef) Thanks [@kyledurand](https://github.com/kyledurand)! - Renamed `color` prop on Text to `tone` - [Shopify#9993](Shopify#9993) [`21c53aeb32`](Shopify@21c53ae) 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` - [Shopify#10232](Shopify#10232) [`eb2c2035c`](Shopify@799ce67) Thanks [@laurkim](https://github.com/laurkim)! - Removed `optionRole` prop from `OptionList` component ### Minor Changes - [Shopify#10238](Shopify#10238) [`b17d23d69`](Shopify@9da2ef5) 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 - [Shopify#10228](Shopify#10228) [`e18ca907e`](Shopify@c1e36bc) Thanks [@gwyneplaine](https://github.com/gwyneplaine)! - `Filters` Remove unused disabled states in `FilterPill` - [Shopify#10268](Shopify#10268) [`dbe3d9ece`](Shopify@8f7721f) Thanks [@laurkim](https://github.com/laurkim)! - Fixed broken focus ring styles on `ResourceItem` component - [Shopify#10238](Shopify#10238) [`b17d23d69`](Shopify@9da2ef5) 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 - [Shopify#10230](Shopify#10230) [`a573e55d0`](Shopify@eacb1fe) Thanks [@gwyneplaine](https://github.com/gwyneplaine)! - `IndexFilter` remove custom `FilterButton` in favor of directly invoking the Polaris `Button` component. - Updated dependencies \[[`86d4040c0`](Shopify@2d31d44)]: - @shopify/polaris-tokens@7.13.0-beta.0 ## @shopify/polaris-tokens@7.13.0-beta.0 ### Minor Changes - [Shopify#10382](Shopify#10382) [`86d4040c0`](Shopify@2d31d44) 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`](Shopify@2d31d44)]: - @shopify/polaris-tokens@7.13.0-beta.0 Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
WHY are these changes introduced?
Fixes #9993 (comment)
Fixes #10104
WHAT is this pull request doing?
https://shopify.chromatic.com/docs/resource-loading#solution-b-check-fonts-have-loaded-in-a-loader
How to 🎩
🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines
Copy-paste this code in
playground/Playground.tsx:🎩 checklist
README.mdwith documentation changes