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.

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

[WIP] dart-sass compatibility tweaks #4298

Closed
wants to merge 3 commits into from
Closed

[WIP] dart-sass compatibility tweaks #4298

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ad86f56
Upgrade to dart-sass
atesgoral Jul 9, 2021
26dedf6
Update Rollup plugin
atesgoral Jul 9, 2021
38d1786
Divison migrations
atesgoral Jul 9, 2021
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
5 changes: 2 additions & 3 deletions config/rollup/plugin-styles.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import path from 'path';
import {promisify} from 'util';

import {createFilter} from '@rollup/pluginutils';
import nodeSass from 'node-sass';
import sass from 'sass';
import postcss from 'postcss';
import cssModules from 'postcss-modules';

Expand All @@ -22,7 +22,7 @@ export function styles({

const filter = createFilter(include, exclude);

const renderSass = promisify(nodeSass.render);
const renderSass = promisify(sass.render);
Copy link
Member

Choose a reason for hiding this comment

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

We will want to use sass.renderSync, which is apparently the faster of the two methods. I did a similar thing in here


const styleProcessor = postcss([
cssModules({
Expand Down Expand Up @@ -147,7 +147,6 @@ export function styles({

const sassOutput = await renderSass({
data: source,
outputStyle: 'compact',
includePaths: [path.dirname(id)],
}).then((result) => result.css.toString());

Expand Down
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@
"marked": "^0.7.0",
"mutationobserver-shim": "^0.3.3",
"node-cmd": "^3.0.0",
"node-sass": "^4.12.0",
"sass": "^1.35.2",
"npm-run-all": "^4.1.5",
"object-hash": "^1.3.1",
"postcss": "^7.0.18",
Expand Down
2 changes: 1 addition & 1 deletion src/components/ActionList/ActionList.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

$image-size: rem(20px);
$item-min-height: rem(40px);
$item-vertical-padding: ($item-min-height - line-height(body)) / 2;
$item-vertical-padding: ($item-min-height - line-height(body)) * 0.5;

.ActionList {
list-style: none;
Expand Down
4 changes: 3 additions & 1 deletion src/components/AppProvider/AppProvider.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
// polaris-react. As the AppProvider is a required wrapper when using polaris
// components then this css shall always be present.

@use "sass:math";

@import '../../styles/common';

:root {
Expand Down Expand Up @@ -32,7 +34,7 @@ button {

html {
position: relative;
font-size: ($base-font-size / $default-browser-font-size) * 100%;
font-size: math.div($base-font-size, $default-browser-font-size) * 100%;
-webkit-font-smoothing: antialiased;

// This needs to come after -webkit-font-smoothing
Expand Down
4 changes: 2 additions & 2 deletions src/components/Avatar/Avatar.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ $large-size: rem(60px);
max-width: 100%;
background: var(--p-surface-neutral);
color: var(--p-icon-subdued);
border-radius: $large-size / 2;
border-radius: $large-size * 0.5;
user-select: none;

@media (forced-colors: active) {
Expand Down Expand Up @@ -78,7 +78,7 @@ $large-size: rem(60px);
width: 100%;
height: 100%;
background-color: var(--p-background);
border-radius: $large-size / 2;
border-radius: $large-size * 0.5;
transform: translate(-50%, -50%);
object-fit: cover;
}
Expand Down
2 changes: 1 addition & 1 deletion src/components/Badge/Badge.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ $horizontal-padding: spacing(tight);
$vertical-padding: rem(3px);

$pip-size: rem(10px);
$pip-spacing: ($height - $pip-size) / 2;
$pip-spacing: ($height - $pip-size) * 0.5;

.Badge {
--p-component-badge-pip-color: var(--p-icon);
Expand Down
2 changes: 1 addition & 1 deletion src/components/Breadcrumbs/Breadcrumbs.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ $icon-size: rem(20px);
color: var(--p-text-subdued);
text-decoration: none;
margin: 0;
padding: spacing(extra-tight) / 2;
padding: spacing(extra-tight) * 0.5;
border-radius: var(--p-border-radius-base);
border: 1px solid var(--p-border-neutral-subdued);
@include focus-ring($border-width: rem(1px));
Expand Down
10 changes: 5 additions & 5 deletions src/components/Button/Button.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
@import '../../styles/common';

$min-height: control-height();
$vertical-padding: ($min-height - line-height(body) - rem(2px)) / 2;
$vertical-padding: ($min-height - line-height(body) - rem(2px)) * 0.5;
$slim-min-height: rem(28px);
$slim-vertical-padding: ($slim-min-height - line-height(body) - rem(2px)) / 2;
$slim-vertical-padding: ($slim-min-height - line-height(body) - rem(2px)) * 0.5;
$large-min-height: rem(44px);
$large-vertical-padding: ($large-min-height - line-height(body) - rem(2px)) / 2;
$large-vertical-padding: ($large-min-height - line-height(body) - rem(2px)) * 0.5;
$spinner-size: rem(20px);

$partial-button-filled-pressed-box-shadow: inset 0 0 0 0 transparent,
Expand Down Expand Up @@ -107,8 +107,8 @@ $stacking-order: (
position: absolute;
top: 50%;
left: 50%;
margin-top: -($spinner-size / 2);
margin-left: -($spinner-size / 2);
margin-top: -($spinner-size * 0.5);
margin-left: -($spinner-size * 0.5);
}

.primary {
Expand Down
4 changes: 2 additions & 2 deletions src/components/CheckableButton/CheckableButton.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@

$button-min-height: control-height();

$button-vertical-padding: ($button-min-height - line-height(body) - rem(2px)) /
2;
$button-vertical-padding: ($button-min-height - line-height(body) - rem(2px)) *
0.5;
$control-size: rem(16px);
$chekbox-label-margin: rem(20px);

Expand Down
4 changes: 3 additions & 1 deletion src/components/ColorPicker/ColorPicker.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
@use "sass:list";

@import '../../styles/common';

$picker-size: rem(160px);
Expand All @@ -15,7 +17,7 @@ $stacking-order: (
var(--p-surface) 0% 25%,
var(--p-surface-neutral-subdued) 0% 50%
)
50% / #{spacing() spacing()};
list.slash(50%, spacing() spacing());
}

.ColorPicker {
Expand Down
4 changes: 3 additions & 1 deletion src/components/DatePicker/DatePicker.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
@use "sass:math";

@import '../../styles/common';

$font-size: rem(12px);
Expand Down Expand Up @@ -35,7 +37,7 @@ $range-end-border-radius: rem(30px);
}

.DayCell {
width: (100% / 7);
width: math.div(100%, 7);
background: transparent;
margin: 0;
padding: 0;
Expand Down
2 changes: 1 addition & 1 deletion src/components/DropZone/components/FileUpload/FileUpload.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

$fileupload-padding: rem(15px);
$slim-min-height: rem(30px);
$slim-vertical-padding: ($slim-min-height - line-height(body) - rem(2px)) / 2;
$slim-vertical-padding: ($slim-min-height - line-height(body) - rem(2px)) * 0.5;

.FileUpload {
padding: $fileupload-padding;
Expand Down
2 changes: 1 addition & 1 deletion src/components/KeyboardKey/KeyboardKey.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ $bottom-shadow-size: 2px;
.KeyboardKey {
display: inline-block;
height: $key-base-height;
margin: 0 (spacing(extra-tight) / 2) $bottom-shadow-size;
margin: 0 (spacing(extra-tight) * 0.5) $bottom-shadow-size;
padding: 0 spacing(extra-tight);
background-color: var(--p-surface);
box-shadow: 0 0 0 1px var(--p-border-subdued),
Expand Down
4 changes: 3 additions & 1 deletion src/components/Layout/Layout.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
@use "sass:math";

@import '../../styles/common';

$secondary-basis: layout-width(secondary, min);
$primary-basis: layout-width(primary, min);
$one-half-basis: layout-width(one-half-width, base);
$one-third-basis: layout-width(one-third-width, base);
$relative-size: $primary-basis / $secondary-basis;
$relative-size: math.div($primary-basis, $secondary-basis);

.Layout {
display: flex;
Expand Down
2 changes: 1 addition & 1 deletion src/components/OptionList/components/Option/Option.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ $control-vertical-adjustment: rem(2px);
min-height: $min-height;
text-align: left;
border-radius: var(--p-border-radius-base);
margin-top: spacing(tight) / 2;
margin-top: spacing(tight) * 0.5;
}

.SingleSelectOption {
Expand Down
4 changes: 2 additions & 2 deletions src/components/Page/components/Header/Header.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ $action-menu-rollup-computed-width: rem(40px);
// stylelint-disable-next-line selector-max-class
.mobileView & {
position: absolute;
top: spacing(loose) + (control-height() / 4);
top: spacing(loose) + (control-height() * 0.25);
right: spacing(loose);
margin-top: 0;

Expand All @@ -136,7 +136,7 @@ $action-menu-rollup-computed-width: rem(40px);

// stylelint-disable-next-line selector-max-class
.mobileView.hasNavigation & {
top: control-height() / 2;
top: control-height() * 0.5;
}

@include print-hidden;
Expand Down
2 changes: 1 addition & 1 deletion src/components/ProgressBar/ProgressBar.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
@function progress-bar-height($height: base) {
$base: 16px;
$data: (
small: rem($base / 2),
small: rem($base * 0.5),
base: rem($base),
large: rem($base * 2),
);
Expand Down
4 changes: 2 additions & 2 deletions src/components/RangeSlider/components/DualThumb/DualThumb.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ $range-output-spacing: rem(16px);
transition-property: opacity, visibility, bottom;
transition-duration: var(--p-duration-1-5-0);
transition-timing-function: var(--p-ease);
transform: translateX(calc(-50% + #{$range-thumb-size / 2}));
transform: translateX(calc(-50% + #{$range-thumb-size * 0.5}));

.Thumbs:hover + &,
.Thumbs:focus + & {
Expand Down Expand Up @@ -169,7 +169,7 @@ $range-output-spacing: rem(16px);
transform: translateY(-$range-output-spacing);

@include page-content-when-not-partially-condensed {
transform: translateY(-($range-output-spacing / 2));
transform: translateY(-($range-output-spacing * 0.5));
}
}
// stylelint-enable selector-max-specificity, selector-max-combinators, selector-max-class
Expand Down
6 changes: 3 additions & 3 deletions src/components/RangeSlider/components/SingleThumb/SingleThumb.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,11 +111,11 @@ $stacking-order: (

&::-ms-thumb {
margin-top: 0;
transform: translateY($range-thumb-size / 5) scale(0.4);
transform: translateY($range-thumb-size * 0.2) scale(0.4);
}

&::-webkit-slider-thumb {
margin-top: -($range-thumb-size - $range-track-height) / 2;
margin-top: -($range-thumb-size - $range-track-height) * 0.5;
}

&:active {
Expand Down Expand Up @@ -216,7 +216,7 @@ $range-output-spacing: rem(16px);
transform: translateY(-$range-output-spacing);

@include page-content-when-not-partially-condensed {
transform: translateY(-($range-output-spacing / 2));
transform: translateY(-($range-output-spacing * 0.5));
}
}
@include high-contrast-outline;
Expand Down
2 changes: 1 addition & 1 deletion src/components/Tabs/Tabs.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

$item-min-height: rem(16px);
$item-min-width: rem(50px);
$item-vertical-padding: $item-min-height / 2;
$item-vertical-padding: $item-min-height * 0.5;
$underline-height: border-width(thicker);
$focus-state-box-shadow-color: rgba(0, 0, 0, 0.8);

Expand Down
8 changes: 5 additions & 3 deletions src/components/VideoThumbnail/VideoThumbnail.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
@use "sass:math";

@import '../../styles/common';
$start-button-size: rem(60px);
$progress-bar-height: rem(6px);

.Thumbnail {
position: relative;
// Accommodating 16:9 responsive block for video
padding-bottom: 9 / 16 * 100%;
padding-bottom: math.div(9, 16) * 100%;
background-size: cover;
background-position: center center;
background-repeat: no-repeat;
Expand Down Expand Up @@ -53,8 +55,8 @@ $progress-bar-height: rem(6px);
left: 50%;
width: rem($start-button-size);
height: rem($start-button-size);
margin-top: rem(-$start-button-size / 2);
margin-left: rem(-$start-button-size / 2);
margin-top: rem(-$start-button-size * 0.5);
margin-left: rem(-$start-button-size * 0.5);
}

.Timestamp {
Expand Down
8 changes: 5 additions & 3 deletions src/styles/foundation/_colors.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
// stylelint-disable-next-line scss/partial-no-import
@use "sass:math";
Copy link
Member

@BPScott BPScott Jul 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This is going to be an issue.

Code in src/components all get compiled and shipped as css so we can author those files in whatever format we want, no problems there.
Code in src/styles (or more strictly, code in files imported from src/styles/_public-api.scss) however is the Polaris public sass API, and is used by consuming apps. This means that any code in these files needs to be understood by consuming apps. Introducing @use - dart-sass only syntax - here is a breaking change as it means that consumers will no longer be able to author app scss in node-sass. We create a hard dependency of:this version of polaris ships mixins with dart-sass specific syntax and thus can only be use in apps that compile their scss with dart-sass.

In order to not require a specific major version of polaris to be tied to a specific version of sewing-kit this either needs to be reworked, and either we remove these utilities from polaris (color() is considered not desired but its usage in consuming apps is pervasive which means it's hard to remove) before performing the migration to dart-sass or we come up with some way to support both node-sass and dart-sass.

On the up-side it's been agreed upon that having a sass API is not desired, and it causes more hassle than desired, however work to actually remove it is slow going.


@import '../polaris-tokens/colors.color-map';

///
Expand Down Expand Up @@ -69,9 +71,9 @@ $color-palette-data: map-extend(
$background: rgb(255, 255, 255);
}

$red: red($background) * red($foreground) / 255;
$green: green($background) * green($foreground) / 255;
$blue: blue($background) * blue($foreground) / 255;
$red: math.div(red($background) * red($foreground), 255);
$green: math.div(green($background) * green($foreground), 255);
$blue: math.div(blue($background) * blue($foreground), 255);

$opacity: opacity($foreground);
$background-opacity: opacity($background);
Expand Down
2 changes: 1 addition & 1 deletion src/styles/foundation/_typography.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ $font-family-data: (
Consolas,
Liberation Mono,
Menlo,
monospace !default},
monospace},
Copy link
Member

Choose a reason for hiding this comment

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

This looks like a straight up bug and we can remove this in a separate pr

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#4308

);

$line-height-data: (
Expand Down
14 changes: 8 additions & 6 deletions src/styles/foundation/_utilities.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
@use "sass:math";
Copy link
Member

Choose a reason for hiding this comment

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

Another case where dart-sass only code is introduced to the public sass API


$default-browser-font-size: 16px;
$base-font-size: 10px;

Expand All @@ -13,9 +15,9 @@ $base-font-size: 10px;
} @else if $unit == 'rem' {
@return $value;
} @else if $unit == 'px' {
@return $value / $base-font-size * 1rem;
@return math.div($value, $base-font-size) * 1rem;
} @else if $unit == 'em' {
@return $unit / 1em * 1rem;
@return math.div($unit, 1em) * 1rem;
} @else {
@error 'Value must be in px, em, or rem.';
}
Expand All @@ -33,9 +35,9 @@ $base-font-size: 10px;
} @else if $unit == 'px' {
@return $value;
} @else if $unit == 'em' {
@return ($value / 1em) * $base-font-size;
@return math.div($value, 1em) * $base-font-size;
} @else if $unit == 'rem' {
@return ($value / 1rem) * $base-font-size;
@return math.div($value, 1rem) * $base-font-size;
} @else {
@error 'Value must be in rem, em, or px.';
}
Expand All @@ -54,9 +56,9 @@ $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);
@return math.div($value, 1rem) * 1em * math.div($base-font-size, $default-browser-font-size);
} @else if $unit == 'px' {
@return $value / $default-browser-font-size * 1em;
@return math.div($value, $default-browser-font-size) * 1em;
} @else {
@error 'Value must be in px, rem, or em.';
}
Expand Down
2 changes: 1 addition & 1 deletion src/styles/shared/_buttons.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@

@mixin button-base {
$min-height: control-height();
$vertical-padding: ($min-height - line-height(body) - rem(2px)) / 2;
$vertical-padding: ($min-height - line-height(body) - rem(2px)) * 0.5;

@include recolor-icon(var(--p-icon));
@include focus-ring($border-width: border-width('base'));
Expand Down
2 changes: 1 addition & 1 deletion src/styles/shared/_controls.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
}

@function control-vertical-padding() {
@return (control-height() - line-height(input) - rem(2px)) / 2;
@return (control-height() - line-height(input) - rem(2px)) * 0.5;
}

@function control-icon-transition() {
Expand Down
Loading