Skip to content
This repository was archived by the owner on Sep 30, 2025. It is now read-only.

Conversation

sam-b-rose
Copy link
Member

@sam-b-rose sam-b-rose commented Oct 25, 2022

WHY are these changes introduced?

Resolves #6588, resolves #7192, resolves #7584, resolves #7047, resolves #7197, resolves #7048, resolves #7199, resolves #7206, resolves #7205, resolves #7598, resolves #7583.

WHAT is this pull request doing?

Replaces the old typography components with the new Text component. These changes where created using the replace-text-component migration:

polaris-migrator replace-text-component "./polaris-react/src/**/*.tsx" --relative

Additionally, manual updates for the following text mixins have been updated to either use the Text component or the token inside the classes directly:

  • text-style-body
  • text-style-heading
  • text-style-subheading
  • text-style-caption
  • text-style-button
  • text-style-button-large
  • text-emphasis-subdued
  • text-emphasis-strong
  • nav-item-text-attributes

@github-actions
Copy link
Contributor

github-actions bot commented Oct 25, 2022

size-limit report 📦

Path Size
polaris-react-cjs 209.62 KB (+0.19% 🔺)
polaris-react-esm 136.04 KB (+0.31% 🔺)
polaris-react-esnext 191.16 KB (-0.02% 🔽)
polaris-react-css 41.33 KB (-0.94% 🔽)

@sam-b-rose sam-b-rose requested review from aveline and laurkim October 26, 2022 19:06
@laurkim
Copy link
Contributor

laurkim commented Oct 26, 2022

This is blocked until we add responsive styling to the Text component to accommodate the responsive styling that DisplayText has. I'll link the PR for that functionality once I have it up.

@sarahill
Copy link
Contributor

sarahill commented Oct 26, 2022

@samrose3 there are a few components that need to be manually updated because they don't follow the mappings 1:1. There are details in individual issues for each component. Links to those issues can be found in #6794. I'll list them here too:

There's details on why and what they should look like. There are some cases where we needed to make adjustments for smaller screens others where the styles just needed to be adjusted to visually work with the updated styles. Happy to chat more about this. Let me know how I can help. Thanks!

@laurkim
Copy link
Contributor

laurkim commented Oct 26, 2022

This is blocked until we add responsive styling to the Text component to accommodate the responsive styling that DisplayText has. I'll link the PR for that functionality once I have it up.

PR to update responsive styling

@laurkim laurkim force-pushed the replace-typography-components-with-text branch from 6a5ff90 to 2f9e07a Compare October 27, 2022 12:12
@laurkim
Copy link
Contributor

laurkim commented Oct 27, 2022

@samrose3 I rebased off latest main to pick up recent changes to Text that added responsive styling.

@aveline
Copy link
Contributor

aveline commented Oct 27, 2022

There are 86 changes in Chromatic, appears to be changes in font sizes. Are these expected? Have they all been reviewed? If so, they should also be accepted.

Personally some of them don't look right to me. In some cases if the larger font size is expected, then I think spacing needs to be adjusted.

cc @sarahill @nayeobkim

@sarahill
Copy link
Contributor

sarahill commented Oct 27, 2022

Just chatted with @laurkim and she's going to add responsive styling for HeadingXl and HeadingLg. I think this should take care of some of the regressions. There will still be some because of the weight changes which are expected.

I think Lo mentioned the mapping will also need to be updated because it was a little off.

@aveline @laurkim Does it make sense to wait to go through the Chromatic changes until the responsive PR is merged and the mappings are updated?

Also, we decided to hold off on those manual updates until we QA the current state with the mappings updated then we can make a call on the priority of those manual updates.

@aveline
Copy link
Contributor

aveline commented Oct 27, 2022

#7530 (comment) Looks like this should now include the responsive styles, but yes wait until the mappings are updated before going through Chromatic.

@sam-b-rose sam-b-rose force-pushed the replace-typography-components-with-text branch from a5a7a37 to 6325a53 Compare October 28, 2022 16:12
@sam-b-rose
Copy link
Member Author

I've updated the Display Text variant mapping and incorporated the responsive style changes. Everything should be g2g and ready for a code and visual review!

Copy link
Contributor

@sarahill sarahill left a comment

Choose a reason for hiding this comment

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

One thing I'm noticing as I'm reviewing Chromatic is that text using body-md seems to be going up to 15px on breakpoint-xs and breakpoint-sm. 15px isn't a value in our type scale.

The intent is that the default style, body-md, would stay the same across screen sizes by default. The exceptions are:

  • Heading4xl, Heading3xl, Heading2xl, HeadingXl, HeadingLg
  • and input fields should use size 16px with line-height 20px on breakpoint-xs and sm

☝️ These are looking as expected. It's just body-md that's using 15px.

Let me know if you have questions. Happy to chat more and sorry for all the back and forth.

@samrose3 @alex-page is this in scope or is the intent to hardcode values, for now, to keep visual changes minimal? I'm unclear on the approach and it's making it hard to QA. I could be misremembering and if you can help me with getting some clarity I would appreciate it :)

@laurkim
Copy link
Contributor

laurkim commented Oct 31, 2022

One thing I'm noticing as I'm reviewing Chromatic is that text using body-md seems to be going up to 15px on breakpoint-xs and breakpoint-sm. 15px isn't a value in our type scale.

@sarahill this is probably because there are classNames wrapping the text component with the text-style-body mixin. I can take another pass to remove any I find.

Copy link
Member

@kyledurand kyledurand left a comment

Choose a reason for hiding this comment

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

This is huge, great job all. There are a few comments but nothing super pressing, except maybe background-color: green on the Select label. I'd like to fix up some of the nested element selectors where possible

Copy link
Member Author

@sam-b-rose sam-b-rose left a comment

Choose a reason for hiding this comment

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

Awesome, work! Looks good to me.

There seem to be a few changes unrelated to the Text and text style mixins migration, however I'm comfortable with them as long as we have a thorough design review 👍

Copy link
Contributor

@sarahill sarahill left a comment

Choose a reason for hiding this comment

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

🎉 🚀 🥳

Copy link
Contributor

@aveline aveline left a comment

Choose a reason for hiding this comment

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

Nice work!

@laurkim
Copy link
Contributor

laurkim commented Nov 2, 2022

/snapit

@laurkim laurkim requested a review from kyledurand November 2, 2022 19:28
@github-actions
Copy link
Contributor

github-actions bot commented Nov 2, 2022

🫰✨ Thanks @laurkim! Your snapshots have been published to npm.

Test the snapshots by updating your package.json with the newly published versions:

yarn add @shopify/plugin-polaris@0.0.0-snapshot-release-20221102192934
yarn add @shopify/polaris-icons@0.0.0-snapshot-release-20221102192934
yarn add @shopify/polaris-migrator@0.0.0-snapshot-release-20221102192934
yarn add @shopify/polaris@0.0.0-snapshot-release-20221102192934
yarn add @shopify/stylelint-polaris@0.0.0-snapshot-release-20221102192934

Copy link
Member

@kyledurand kyledurand left a comment

Choose a reason for hiding this comment

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

👏

@laurkim laurkim merged commit 79d92a8 into main Nov 3, 2022
@laurkim laurkim deleted the replace-typography-components-with-text branch November 3, 2022 14:37
@github-actions github-actions bot mentioned this pull request Nov 3, 2022
laurkim added a commit that referenced this pull request Nov 3, 2022
### WHY are these changes introduced?

Resolves #6588.

### WHAT is this pull request doing?

Runs the `replace-text-component` migration without the relative flag to
update and replace instances where text components are imported from
`@shopify/polaris` in the `polaris-react` directory.

Related to the [PR](#7530) that
replaces relative imports.

Co-authored-by: Sam Rose <sam.rose@shopify.com>
sam-b-rose added a commit that referenced this pull request Nov 3, 2022
* main:
  Add flex properties to Inline, use logical property instead of alignY (#7633)
  [polaris.shopify.com] Replace typography with Text component (#7634)
  Add constraints to `stylelint-polaris/coverage` disable comments (#7589)
  Check for target component in Text migrations before modifying file (#7632)
  Replace typography component in stories with Text (#7572)
  Inline remove wrap children in div (#7625)
  Replace typography components with Text (#7530)
  [Layout foundations] Update `AlphaCard` component docs and guidance (#7568)
laurkim pushed a commit that referenced this pull request Nov 4, 2022
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 main, this PR will
be updated.


# Releases
## @shopify/polaris-icons@6.5.0

### Minor Changes

- [#7548](#7548)
[`432bdd5fe`](432bdd5)
Thanks [@anthonymenecola](https://github.com/anthonymenecola)! - add
cancel major icon


- [#7620](#7620)
[`35be8a003`](35be8a0)
Thanks [@rdott](https://github.com/rdott)! - Added inactive location
minor and major icons

## @shopify/polaris-migrator@0.8.0

### Minor Changes

- [#7403](#7403)
[`8859f5db5`](8859f5d)
Thanks [@jesstelford](https://github.com/jesstelford)! - Introduce
`migrate-motion` migration for migrating `transition`,
`transition-duration`, and `transition-delay` usages of duration values.


- [#7606](#7606)
[`cf7badbd1`](cf7badb)
Thanks [@samrose3](https://github.com/samrose3)! - Renamed and split
migrations based on scope and type (react, scss, and styles)


- [#7543](#7543)
[`8c1989618`](8c19896)
Thanks [@jesstelford](https://github.com/jesstelford)! - Expose
utilities for SASS Migrations to leverage the Suggestion-on-partial-fix
pattern


- [#7529](#7529)
[`3652eb901`](3652eb9)
Thanks [@samrose3](https://github.com/samrose3)! - Add relative option
for replace-text-component migration


- [#7532](#7532)
[`ba576498d`](ba57649)
Thanks [@jesstelford](https://github.com/jesstelford)! - Expose the
.report() method to SASS migrations for easier aggregation of discovered
issues during a migration run.

### Patch Changes

- [#7606](#7606)
[`cf7badbd1`](cf7badb)
Thanks [@samrose3](https://github.com/samrose3)! - Update
`createInlineComment` to format text with RegExp


- [#7606](#7606)
[`cf7badbd1`](cf7badb)
Thanks [@samrose3](https://github.com/samrose3)! - Add support to
replace Identifiers along with JSXIdentifiers for Text migration


- [#7632](#7632)
[`1f2ec8bfe`](1f2ec8b)
Thanks [@samrose3](https://github.com/samrose3)! - Check for targeted
component import before modifying in Text component migration

- Updated dependencies
\[[`6e9edd3b5`](6e9edd3)]:
    -   @shopify/polaris-tokens@6.3.0

## @shopify/polaris@10.11.0

### Minor Changes

- [#7572](#7572)
[`20c8cad81`](20c8cad)
Thanks [@laurkim](https://github.com/laurkim)! - Replaced usage of text
components in component stories with `Text` component


- [#7621](#7621)
[`6e9edd3b5`](6e9edd3)
Thanks [@aveline](https://github.com/aveline)! - - Added border width
prop to `Box`
- Exported color token subset alias types from tokens package and remove
from `Box`


- [#7068](#7068)
[`ccdcea22e`](ccdcea2)
Thanks [@laurkim](https://github.com/laurkim)! - Deprecated
`DisplayText`, `Heading`, `Subheading`, `Caption`, `TextStyle`, and
`VisuallyHidden` components

### Patch Changes

- [#7644](#7644)
[`b3e73ee04`](b3e73ee)
Thanks [@kyledurand](https://github.com/kyledurand)! - Added horizontal
spacing defaults to `Bleed`


- [#7530](#7530)
[`79d92a820`](79d92a8)
Thanks [@samrose3](https://github.com/samrose3)! - Replaced all
typography components with the new `Text` component.
    Added support for `text-inverse` color type on `Text`.
Removed references to the following mixins to use the new `Text` or
tokens directly in classes: `text-style-body`, `text-style-heading`,
`text-style-subheading`, `text-style-caption`, `text-style-button`,
`text-style-button-large`, `text-emphasis-subdued`,
`text-emphasis-strong`, `nav-item-text-attributes`.


- [#7577](#7577)
[`db951f855`](db951f8)
Thanks [@RickyMarou](https://github.com/RickyMarou)! - Page component:
display subtitle even when it's the only header prop set


- [#7633](#7633)
[`1364be7f1`](1364be7)
Thanks [@kyledurand](https://github.com/kyledurand)! - Renamed `alignY`
prop to `alignBlock` on `Inline`
    Added more flex properties to `align` on `Inline`


- [#7443](#7443)
[`7a6fb7c1c`](7a6fb7c)
Thanks [@iAmNathanJ](https://github.com/iAmNathanJ)! - Improve
performance of the Scrollable component with React 18


- [#7625](#7625)
[`9f8b651dd`](9f8b651)
Thanks [@kyledurand](https://github.com/kyledurand)! - Removed wrap
children with div from Inline component


- [#7593](#7593)
[`addd6bcdd`](addd6bc)
Thanks [@kyledurand](https://github.com/kyledurand)! - Improved comments
across layout components, changed default spacing of Inline component to
match AlphaStack


- [#7600](#7600)
[`f006509be`](f006509)
Thanks [@billycai](https://github.com/billycai)! - Add spacing between
title and metadata for Page component


- [#7563](#7563)
[`a9051d678`](a9051d6)
Thanks [@chazdean](https://github.com/chazdean)! - Updated `Inline`
component docs and default prop values


- [#7635](#7635)
[`3cb5377a6`](3cb5377)
Thanks [@iAmNathanJ](https://github.com/iAmNathanJ)! - Fixed Scrollable
component to match existing onScrolledToBottom logic

- Updated dependencies
\[[`432bdd5fe`](432bdd5),
[`6e9edd3b5`](6e9edd3),
[`35be8a003`](35be8a0)]:
    -   @shopify/polaris-icons@6.5.0
    -   @shopify/polaris-tokens@6.3.0

## @shopify/polaris-tokens@6.3.0

### Minor Changes

- [#7621](#7621)
[`6e9edd3b5`](6e9edd3)
Thanks [@aveline](https://github.com/aveline)! - - Added border width
prop to `Box`
- Exported color token subset alias types from tokens package and remove
from `Box`

## @shopify/stylelint-polaris@4.4.0

### Minor Changes

- [#7551](#7551)
[`d7dc4436f`](d7dc443)
Thanks [@aaronccasanova](https://github.com/aaronccasanova)! - Add
`stylelint-polaris/coverage` rule

### Patch Changes

- [#7589](#7589)
[`b7b0ef5a9`](b7b0ef5)
Thanks [@aaronccasanova](https://github.com/aaronccasanova)! - Add
constraints to `stylelint-polaris/coverage` disable comments

- Updated dependencies
\[[`6e9edd3b5`](6e9edd3)]:
    -   @shopify/polaris-tokens@6.3.0

## @shopify/plugin-polaris@0.0.16

### Patch Changes

- Updated dependencies
\[[`8859f5db5`](8859f5d),
[`cf7badbd1`](cf7badb),
[`cf7badbd1`](cf7badb),
[`cf7badbd1`](cf7badb),
[`8c1989618`](8c19896),
[`3652eb901`](3652eb9),
[`1f2ec8bfe`](1f2ec8b),
[`ba576498d`](ba57649)]:
    -   @shopify/polaris-migrator@0.8.0

## polaris.shopify.com@0.24.0

### Minor Changes

- [#7068](#7068)
[`ccdcea22e`](ccdcea2)
Thanks [@laurkim](https://github.com/laurkim)! - Deprecated
`DisplayText`, `Heading`, `Subheading`, `Caption`, `TextStyle`, and
`VisuallyHidden` pages and removed examples


- [#7609](#7609)
[`343865159`](3438651)
Thanks [@sarahill](https://github.com/sarahill)! - Added new type style
guidance and info to typography docs

### Patch Changes

- [#7634](#7634)
[`4db441756`](4db4417)
Thanks [@laurkim](https://github.com/laurkim)! - Replaced usage of
typography components (`DisplayText`, `Heading`, `Subheading`,
`Caption`, `VisuallyHidden`, `TextStyle`) with the new `Text` component


- [#7604](#7604)
[`aa82c82ff`](aa82c82)
Thanks [@chazdean](https://github.com/chazdean)! - Updated `Inline`
component doc vertical alignment example


- [#7568](#7568)
[`ab0cf251f`](ab0cf25)
Thanks [@chazdean](https://github.com/chazdean)! - Updated `AlphaCard`
component guidance and examples


- [#7633](#7633)
[`1364be7f1`](1364be7)
Thanks [@kyledurand](https://github.com/kyledurand)! - Renamed `alignY`
prop to `alignBlock` on `Inline`
    Added more flex properties to `align` on `Inline`


- [#7527](#7527)
[`924e9e5cd`](924e9e5)
Thanks [@chazdean](https://github.com/chazdean)! - Update `Columns`
component docs


- [#7596](#7596)
[`749ee31ee`](749ee31)
Thanks [@kyledurand](https://github.com/kyledurand)! - Fixed home promo
image layout


- [#7563](#7563)
[`a9051d678`](a9051d6)
Thanks [@chazdean](https://github.com/chazdean)! - Updated `Inline`
component docs and default prop values


- [#7566](#7566)
[`567822218`](5678222)
Thanks [@kyledurand](https://github.com/kyledurand)! - Bumped nextjs


- [#7602](#7602)
[`9931ce0b4`](9931ce0)
Thanks [@kyledurand](https://github.com/kyledurand)! - Bumped nextjs to
13.0.1


- [#7571](#7571)
[`4c5ccc8fa`](4c5ccc8)
Thanks [@chazdean](https://github.com/chazdean)! - Updated `Bleed`
component guidance and examples

- Updated dependencies
\[[`b3e73ee04`](b3e73ee),
[`20c8cad81`](20c8cad),
[`79d92a820`](79d92a8),
[`db951f855`](db951f8),
[`432bdd5fe`](432bdd5),
[`6e9edd3b5`](6e9edd3),
[`ccdcea22e`](ccdcea2),
[`1364be7f1`](1364be7),
[`7a6fb7c1c`](7a6fb7c),
[`9f8b651dd`](9f8b651),
[`addd6bcdd`](addd6bc),
[`f006509be`](f006509),
[`a9051d678`](a9051d6),
[`3cb5377a6`](3cb5377),
[`35be8a003`](35be8a0)]:
    -   @shopify/polaris@10.11.0
    -   @shopify/polaris-icons@6.5.0
    -   @shopify/polaris-tokens@6.3.0

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@alex-page alex-page restored the replace-typography-components-with-text branch December 8, 2022 01:31
@alex-page alex-page deleted the replace-typography-components-with-text branch December 8, 2022 01:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
6 participants