-
Notifications
You must be signed in to change notification settings - Fork 216
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
WIP | feat: update css to latest #5142
base: main
Are you sure you want to change the base?
Conversation
|
Branch previewReview the following VRT differencesWhen a visual regression test fails (or has previously failed while working on this branch), its results can be found in the following URLs:
If the changes are expected, update the |
Tachometer resultsCurrently, no packages are changed by this PR... |
Lighthouse scores
What is this?Lighthouse scores comparing the documentation site built from the PR ("Branch") to that of the production documentation site ("Latest") and the build currently on Transfer Size
Request Count
|
1987d3a
to
d74a21d
Compare
eaebcbe
to
8dd0fe4
Compare
> | ||
Enable | ||
</sp-button> | ||
<sp-button-group slot="button"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The buttons are not correctly spaced in the slot without the button-group wrapper so this seemed like a good update (especially in terms of demonstrating best practices to customers).
package.json
Outdated
@@ -385,7 +388,7 @@ | |||
] | |||
}, | |||
"process-spectrum": { | |||
"command": "node ./scripts/spectrum-vars.js && node ./tasks/process-spectrum.js && node ./scripts/generate-tokens.js && yarn lint:css --fix && pretty-quick --pattern \"{packages,tools}/**/*.css\" && pretty-quick --pattern \"packages/dialog/src/spectrum-dialog.css\"", | |||
"command": "node ./scripts/spectrum-vars.js && node ./tasks/process-spectrum.js && node ./scripts/generate-tokens.js && yarn lint:css --fix && pretty-quick --pattern \"{packages,tools}/**/*.css\"", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the additional pretty-quick run against spectrum-dialog because it's already included in the pattern for the previous pretty-quick run.
.prettierrc.yaml
Outdated
@@ -6,3 +6,7 @@ trailingComma: es5 | |||
bracketSpacing: true | |||
arrowParens: always | |||
htmlWhitespaceSensitivity: ignore | |||
overrides: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prettier's line wrapping for CSS does not have exceptions for var functions; adding whitespace or newlines to var functions cannot be minified safely downstream (and many minifiers won't) because whitespace is valid in a CSS var function. By increasing the line-length for CSS, we prevent invalid newlines being adding into var functions.
package.json
Outdated
"spectrum-css": "wireit", | ||
"spectrum-tokens": "wireit", | ||
"spectrum-vars": "wireit", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These commands don't exist in wireit and haven't for a long while - seems safe to remove the alias'es right?
@@ -12,6 +12,7 @@ | |||
"analyze": "lit-analyzer \"{packages,tools}/*/src/**/!(*.css).ts\"", | |||
"build": "wireit", | |||
"build:clear-cache": "rimraf packages/*/tsconfig.tsbuildinfo && rimraf tools/*/tsconfig.tsbuildinfo", | |||
"build:component:css": "node ./tasks/process-spectrum.js", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted the ability to force-run the build only for specific components as a means of validating updates to the spectrum-config files. This has been documented in the README.
package.json
Outdated
"peerDependencies": { | ||
"common-tags": "^1.8.0" | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This implies consumers checking out the monorepo root would need to install common-tags locally for themselves in order to be able to build this project but I don't believe that is the case. Can we remove this? I don't think the monorepo root has any required peerDeps.
f339cb3
to
b61baca
Compare
…eserve browser support
b61baca
to
f518619
Compare
f518619
to
d5fea20
Compare
Description
This is a full upgrade of all packages to use the latest published CSS from the @spectrum-css project.
Why update everything at once?
It is often easier to see changes in components that influence one-another (i.e., --mod passthrough styles from parent components) if we bump all components together.
That does not mean that all components need to be merged via this PR but rather, that all components can be viewed in unison via this PR.
Related issue(s)
Most related issues exist in Jira but this is a nice view of the scope of updates we are still needing in order to be up-to-date with the latest fast-follows design updates and CSS theming optimizations (these optimizations have removed hundreds of extemporaneous custom properties.
How has this been tested?
Strongly recommend relying heavily on the VRT results and sanity-checking the Storybook pages by clicking through some of the more problematic components (especially those with nested SWC components or styles from multiple CSS packages).
Strongly suggest design review this Storybook for their most important components and validate the S2 versions are as they wish. This PR is up-to-date with the latest fast-follows that have been merged.
Did it pass in Desktop?
Did it pass in Mobile?
Did it pass in iPad?
Screenshots (if appropriate)
Types of changes
Checklist
Best practices
This repository uses conventional commit syntax for each commit message; note that the GitHub UI does not use this by default so be cautious when accepting suggested changes. Avoid the "Update branch" button on the pull request and opt instead for rebasing your branch against
main
.