[v4] Don’t try to size html to 10px

[ry5n]

WHY are these changes introduced?

Fixes #431

TL;DR

Polaris sets the font-size on the html element to 62.5% (10px). We do this as a developer convenience, but it causes most elements to be larger than intended in some locales.

Long version

Polaris uses rem units in order to support people who choose a larger or smaller font size at the browser level. However, Shopifolk are used to thinking in px. When we inspect CSS in the browser, the rem output is harder to make sense of: how many px is 0.875rem? But when we set the html element to a font-size of 10px, it’s easier: 1.4rem is 14px.

However, languages that use ideographic writing systems (e.g. Chinese, Korean, Japanese) need larger font sizes for legibility, so browsers like Chrome have a minimum font size of 12px. This means the font size of html computes to 12px, not 10, and many elements get rendered larger than intended.

WHAT is this pull request doing?

This PR removes a small amount of developer convenience in favor of a UI that is sized the same across locales. This PR should not affect the size of rendered text, only the size of interface elements.

HOW does this PR work?

Because we’re now going with the browser default as our root em, we had some redundant code. The code has been refactored and improved to handle conversions involving ems more accurately, fix a bug in one code path, and be (hopefully) more readable.

How to 🎩

Part 1

1. In Chrome, go to Chrome > Preferences
2. In Appearance section, click “Customize fonts”
3. For “Minimum font size”, change the value from 10px (default) to 12px
4. Log into a test Shopify store (production)
5. Verify that elements are larger than normal, but text sizes are normal (body font size should still be 14px)

Part 2

1. Run the local Storybook for this repo, or open it from the Deployment link in this PR
2. With the browser still set to 12px min font-size, view several components
3. Verify that padding, margin, heights and other dimensions are now back to normal
4. Verify that text is not smaller than normal (body font size should still be 14px)

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

🎩 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

[BPScott]

I'm happy that this is happening but it's a terrifying change as it is very fundamental and I'd be a little scared to merge this into master as I don't think "you need to sweep your codebase for references to rem values and adjust them" is an acceptable ask for a minor release. Can we target the version-4.0.0 branch instead?

Can we also just remove this line if we're going for 16px which is the browser default:

polaris-react/src/styles/global/elements.scss

Line 19 in 83fb11a

font-size: ($base-font-size / $default-browser-font-size) * 100%;

[alex-page]

Great work Ryan, this is a big win for internationalisation. Weirdly enough I usually assume 1rem = 16px so this is actually less confusing for me.

As Ben said above there is a bit of redundancy across the code as we are now making the base font size 16px. We can remove the $default-browser-font-size and the $font-size calculation. We can make $font-size: 16px; and that should trickle through the system correctly.

One thing I was unsure about is making the padding 16px on the sheet examples. I might not understand what happens to the CSS that gets passed to these components from props. If it isn't getting converted to REM it should just use 1rem value so that the padding scales when the root font size is increased (e.g. text zoom).

I agree with Ben that I would prefer this change to be made in version 4.0.0, it will likely be a breaking change for some users if they don't change their values or are using variables that are removed.

[ry5n]

Can we target the version-4.0.0 branch instead?

That’s a good point—people using raw rems would be affected and that definitely means this should go into 4.0.

there is a bit of redundancy across the code

When I change this to target 4.0, I’ll look into cleaning this up.

One thing I was unsure about is making the padding 16px on the sheet examples.

This is an artifact of how our examples work. It’s hard to include Sass (or CSS for that matter) in the examples, since they’re just text in a markdown file. The Sheet component has no content of its own, so we’re writing more custom markup than usual, and sprinkling some CSS into style attributes. Those styles were using raw rems, which broke in exactly the way Ben was afraid of. I could re-write these with rems based on 16px, but the thought was that px would be more reliable.

[ry5n]

I’ve stripped out some of the redundant code and changed the base branch to version 4.

In refactoring the unit conversion calculations:

• I found at least one bug—hopefully fixed now. I don’t see any tests, which is slightly scary 😄
• The conversion functions didn’t accurately convert ems. In contexts where 1em ≠ 1rem, devs can now provide an optional second parameter with the current font size to get an accurate em value.
• I found some calculations hard to follow. I’ve reformatted the calculations so they’re hopefully easier to understand. Remember stoichiometry from high school? 🧪

[alex-page]

This looks good @ry5n, nice work. I did a quick top hat over the components in firefox. No changes have been seen in percy so this should be safe to merge.

I think the variable as $root-font-size is fine. Only !default values are encouraged to be overridden in sass.

The bug fix looks good as well, it would be cool to write some tests for this function. Happy to help out, it might be better as another PR to separate concerns.

[kaelig]

Fixing this bug means we'll now have to support it.

Since we know nobody is using this (or they'd have submitted an issue), I vote for removing this part completely:

- } @else if $unit == 'em' {
-    @return $unit / 1em * 1rem;

# and also:
-   @error 'Value must be in px, em, or rem.';
+   @error 'Value must be in px, or rem.';

[ry5n]

The thing is we’re supporting this API already, and fixing it is low cost.

I’d be all for removing em unit conversion if it wasn’t needed. However, we convert to ems in our own code, so we have to support that. The most we could do is remove converting from em units. That seems like a strangely asymmetric API to me.

Is there really any downside to just shipping this bug fix?

[kaelig]

Good point about the asymmetric API, but at the same time we have evidence that a piece of code is not being used.

I'm not going to block this PR from being merged because of this detail, but I'd like us to consider how we can try to ship things that are needed, rather than things we might need.

[ry5n]

Thanks Kaelig. I see where you’re coming from, and I appreciate it. In general I definitely support not adding (and deleting!) code we don’t use.