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.

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

[DO NOT MERGE] Remove SASS CSS diff #11718

Closed
wants to merge 1 commit into from

Conversation

jesstelford
Copy link
Contributor

@jesstelford jesstelford commented Mar 12, 2024

See #11677

@jesstelford jesstelford changed the base branch from main to remove-sass-main-css March 12, 2024 04:29
@@ -3369,6 +3398,8 @@ html[class~='Polaris-Safari-16-Font-Optical-Sizing-Patch'] {
border-bottom: var(--p-border-width-050) dotted var(--p-color-border);
}
.Polaris-Popover {
--pc-popover-visible-portion-of-arrow: 0.3125rem;
--pc-popover-vertical-motion-offset: -0.3125rem;
Copy link
Contributor Author

@jesstelford jesstelford Mar 12, 2024

Choose a reason for hiding this comment

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

SASS variables that were only used locally within a component have been converted to CSS Custom Properties with the --pc- prefix.

--pg-control-height: 2rem;
--pg-control-vertical-padding: calc(
(2.25rem - var(--p-font-line-height-600) - var(--p-space-050)) / 2
);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any SASS variables that couldn't be inlined have been converted to CSS Custom Properties with the --pg- prefix (and that prefix has been added to our linter along with --pc-).

@@ -666,7 +689,7 @@ body {
min-height: 100%;
margin: 0;
padding: 0;
background-color: #f1f2f4;
background-color: rgba(241, 242, 244, 1);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

SASS was previously converting rgb/rgba usages into hex. Postcss does not do this, so the output is closer to the input (ie; identical).

@@ -5576,6 +5606,9 @@ li:first-of-type > .Polaris-Listbox-TextOption {
border: var(--p-border-width-100) solid transparent;
}
}
.Polaris-RadioButton__Backdrop {
position: relative;
}
Copy link
Contributor Author

@jesstelford jesstelford Mar 12, 2024

Choose a reason for hiding this comment

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

The way SASS resolves mixins & nested rules is mostly the same as how postcss-mixins and postcss-nesting does it, except for the case where a mixin comes after a nested rule; the final CSS output will be in a slightly different order (see this explanation for details).

This is only an issue if any declarations following the mixin (which follows a nested rule) are used to overwrite declarations the mixin inserts.

Thankfully, there's an easy fix which requires moving the mixin call above any nested rules. We only had to do this twice (for Navigation & ActionList), the rest we left the source alone and hence the slighly changed output css above.

);
--pc-action-list-item-vertical-padding: calc(
(var(--pc-action-list-item-min-height) - var(--p-font-line-height-500)) / 2
);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Similar to this comment, we had to rearrange the mixins to be at the top here, but due to git's diff algorithm it looks like the CSS Custom Properties have moved.

@@ -12223,14 +12294,14 @@ li:first-of-type > .Polaris-Listbox-TextOption {
.Polaris-Navigation__SecondaryActions {
display: flex;
align-items: center;
height: calc(nav(mobile-height) - var(--p-space-100));
height: calc(var(--pc-navigation-mobile-height) - var(--p-space-100));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this fixing a bug? Looks like it might be!

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant