Skip to content

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

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Jun 13, 2019
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2b8941c
Don’t try to size html to 10px
ry5n May 28, 2019
e7831f6
Remove API for more accurate em conversions
ry5n Jun 11, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions UNRELEASED-V4.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ Use [the changelog guidelines](https://git.io/polaris-changelog-guidelines) to f
### Bug fixes

- Updated translation.yml with the new locales path ([#1649](https://github.com/Shopify/polaris-react/pull/1649))
- Fixed a bug that affected sizing and spacing of elements in some writing systems (notably Chinese, Japanese, and Korean) ([#1590](https://github.com/Shopify/polaris-react/pull/1590))
- Fixed a bug when converting `em` units to `rem` units ([#1590](https://github.com/Shopify/polaris-react/pull/1590))

### Documentation

Expand Down
8 changes: 4 additions & 4 deletions src/components/Sheet/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ class SheetExample extends React.Component {
: null;

return (
<div style={{maxHeight: '640px', overflow: 'visible'}}>
<div style={{maxHeight: '40rem', overflow: 'visible'}}>
<AppProvider
theme={theme}
i18n={{
Expand Down Expand Up @@ -205,7 +205,7 @@ class SheetExample extends React.Component {
borderBottom: '1px solid #DFE3E8',
display: 'flex',
justifyContent: 'space-between',
padding: '1.6rem',
padding: '1rem',
width: '100%',
}}
>
Expand All @@ -217,7 +217,7 @@ class SheetExample extends React.Component {
plain
/>
</div>
<Scrollable style={{padding: '1.6rem', height: '100%'}}>
<Scrollable style={{padding: '1rem', height: '100%'}}>
<ChoiceList
title="Select a sales channel"
name="salesChannelsList"
Expand All @@ -234,7 +234,7 @@ class SheetExample extends React.Component {
borderTop: '1px solid #DFE3E8',
display: 'flex',
justifyContent: 'space-between',
padding: '1.6rem',
padding: '1rem',
width: '100%',
}}
>
Expand Down
37 changes: 23 additions & 14 deletions src/styles/foundation/utilities.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
$default-browser-font-size: 16px;
$base-font-size: 10px;
$root-font-size: 16px;

/// Returns the value in rem for a given pixel value.
/// @param {Number} $value - The pixel value to be converted.
/// Returns the value in rem for a given length value.
/// Note: converting ems only works for elements that have had no font-size changes.
/// @param {Number} $value - The value (em, rem, or px) to be converted.
/// @return {Number} The converted value in rem.

@function rem($value) {
Expand All @@ -13,15 +13,18 @@ $base-font-size: 10px;
} @else if $unit == 'rem' {
@return $value;
} @else if $unit == 'px' {
@return $value / $base-font-size * 1rem;
// e.g. 16px × 1rem/16px = 1rem
@return $value * (1rem / $root-font-size);
} @else if $unit == 'em' {
@return $unit / 1em * 1rem;
// This is a fudge; it assumes 1em == 1rem
@return $value / 1em * 1rem;
Copy link
Contributor

@kaelig kaelig Jun 12, 2019

Choose a reason for hiding this comment

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

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.';

Copy link
Contributor Author

@ry5n ry5n Jun 12, 2019

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@kaelig kaelig Jun 13, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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.

:shipit:

Copy link
Contributor Author

@ry5n ry5n Jun 13, 2019

Choose a reason for hiding this comment

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

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.

} @else {
@error 'Value must be in px, em, or rem.';
}
}

/// Returns the value in pixels for a given rem value.
/// Returns the value in pixels for a given length value.
/// Note: converting ems only works for elements that have had no font-size changes.
/// @param {Number} $value - The rem value to be converted.
/// @return {Number} The converted value in pixels.

Expand All @@ -33,17 +36,20 @@ $base-font-size: 10px;
} @else if $unit == 'px' {
@return $value;
} @else if $unit == 'em' {
@return ($value / 1em) * $base-font-size;
// This is a fudge; it assumes 1em == 1rem
// e.g. 1em × 16px/em = 16px
@return $value * ($root-font-size / 1em);
} @else if $unit == 'rem' {
@return ($value / 1rem) * $base-font-size;
// e.g. 1rem × 16px/rem = 16px
@return $value * ($root-font-size / 1rem);
} @else {
@error 'Value must be in rem, em, or px.';
}
}

/// Returns the value in ems for a given pixel value. Note that this
/// only works for elements that have had no font-size changes.
/// @param {Number} $value - The pixel value to be converted.
/// Returns the value in ems for a given length value.
/// Note: only works for elements that have had no font-size changes.
/// @param {Number} $value - The value (em, rem, or px) to be converted.
/// @return {Number} The converted value in ems.

@function em($value) {
Expand All @@ -54,9 +60,12 @@ $base-font-size: 10px;
} @else if $unit == 'em' {
@return $value;
} @else if $unit == 'rem' {
@return $value / 1rem * 1em * ($base-font-size / $default-browser-font-size);
// This is a fudge; it assumes 1em == 1rem
@return $value / 1rem * 1em;
} @else if $unit == 'px' {
@return $value / $default-browser-font-size * 1em;
// This is a fudge; it assumes 1em == 1rem
// e.g. 14px × 1em/16px = 0.875em
@return $value * (1em / $root-font-size);
} @else {
@error 'Value must be in px, rem, or em.';
}
Expand Down
2 changes: 1 addition & 1 deletion src/styles/global/elements.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ button {

html {
position: relative;
font-size: ($base-font-size / $default-browser-font-size) * 100%;
font-size: ($root-font-size / 16px) * 100%;
-webkit-font-smoothing: antialiased;

// This needs to come after -webkit-font-smoothing
Expand Down