-
Notifications
You must be signed in to change notification settings - Fork 208
chore: Detect and report missing css styles #3591
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
Conversation
03f6b14 to
e6aefe1
Compare
| `${workspace.targetPath}/**`, | ||
| `${workspace.staticSitePath}/**`, | ||
| `${workspace.generatedTestUtils}/**`, | ||
| `${workspace.generatedPath}/custom-css-properties/**`, |
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.
workspace.generatedPath contains other non-generated files: https://github.com/cloudscape-design/components/tree/main/src/internal/generated
Ideally we should clean up them all (or move to a different path), but not in this PR
| @use '../generated/custom-css-properties' as custom-styles; | ||
|
|
||
| :root { | ||
| --awsui-version-info-#{custom-styles.$awsui-commit-hash}: true; |
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.
Core of this check. We check presence of this CSS var in Javascript. If not found – the styles are missing
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.
so if an app doesn't bundle and server styles but another app on the same page has a similar git commit. it wouldn't be detected.
but I guess this is acceptable, and better than what we have now.
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.
Yes, there is a chance of missing some cases, if they happen to have the same commit
| import { PACKAGE_SOURCE, PACKAGE_VERSION, THEME } from './environment'; | ||
|
|
||
| export const metrics = new Metrics(PACKAGE_SOURCE, PACKAGE_VERSION); | ||
| export const metrics = new Metrics({ packageSource: PACKAGE_SOURCE, packageVersion: PACKAGE_VERSION, theme: THEME }); |
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.
Addressing this follow-up cloudscape-design/component-toolkit#136 (comment)
Yes I know this THEME value does not support vr, but it is fine for this use-case
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.
can you explain a bit more?
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.
Before it was like this
const metrics = new Metrics(PACKAGE_SOURCE, PACKAGE_VERSION);
metrics.initMetrics(theme);This is not safe, because it metrics.initMetrics(theme) sets theme as a global variable inside @cloudscape-design/component-toolkit package
Multiple packages may call the same toolkit instance and the same metrics.initMetrics(theme) will overwrite each other theme.
So, now we finally got the theme properly scoped to each bundle
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3591 +/- ##
=======================================
Coverage 96.59% 96.59%
=======================================
Files 806 807 +1
Lines 23500 23530 +30
Branches 8211 8216 +5
=======================================
+ Hits 22699 22729 +30
Misses 794 794
Partials 7 7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
e6aefe1 to
89568bb
Compare
| } | ||
| } | ||
|
|
||
| export function idleWithDelay(cb: () => void) { |
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.
Exported checkMissingStyles and idleWithDelay as separate functions to allow write simpler unit tests on each part of the functionality
89568bb to
90e811c
Compare
90e811c to
aad7297
Compare
| // Custom Property | ||
| if (prop.startsWith('--')) { | ||
| const valueWithoutPostfix = prop.substring(2, prop.length - 7); | ||
| const valueWithoutPostfix = prop.replace(/^--/, '').replace(/-[\d\w]+$/, ''); |
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 code assumed the hash is always 7 characters, which is not true
6d9e39a to
97c9d30
Compare
97c9d30 to
5236ace
Compare
Description
Added check for this old issue: AWSUI-55709
Since releasing VR only artifacts, a few more teams reported the same. Let's add a check to monitor this.
Related links, issue #, if available: n/a
How has this been tested?
Review checklist
The following items are to be evaluated by the author(s) and the reviewer(s).
Correctness
CONTRIBUTING.md.CONTRIBUTING.md.Security
checkSafeUrlfunction.Testing
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.