-
Notifications
You must be signed in to change notification settings - Fork 199
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
chore: compare compiled output fixes #3616
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 87df661 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
File metricsSummaryTotal size: 2.25 MB* 🎉 No changes detected in any packages * Size is the sum of all main files for packages in the library.* An ASCII character in UTF-8 is 8 bits or 1 byte. |
🚀 Deployed on https://pr-3616--spectrum-css.netlify.app |
809c192
to
05bfea3
Compare
@@ -331,22 +331,16 @@ async function run() { | |||
0 | |||
); | |||
core.setOutput("total-size", headMainSize); | |||
|
|||
if (hasBase) { |
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.
We want to output the has-changed value whether there's a base or not - this ensures has-changed is reflected every time this script is run and the logic matches that used above to report in the comment if changes occurred.
@@ -10,6 +10,12 @@ name: Compare | |||
on: | |||
workflow_call: | |||
inputs: | |||
deploy-message: |
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 new variables are passed to Netlify just like the publish-site script does.
@@ -78,7 +78,6 @@ jobs: | |||
publish-dir: dist | |||
production-branch: main | |||
production-deploy: false | |||
netlify-config-path: ./netlify.toml |
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.
Removing this local configuration because it is more securely managed in the Netlify UI than as a locally committed asset.
@@ -7,6 +7,13 @@ | |||
"options": { | |||
"printWidth": 500 | |||
} | |||
}, | |||
{ |
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.
Standardizing the Github Action spacing to mimic more what we have in use today and to align with a lot of the examples for GitHub Actions that exist.
@@ -102,7 +102,7 @@ | |||
}, | |||
"compare": { | |||
"cache": true, | |||
"dependsOn": ["build"], | |||
"dependsOn": ["build", { "projects": ["bundle"], "target": "build" }], |
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 compare tool uses the bundle to style the interface.
@@ -306,7 +300,6 @@ async function processComponent( | |||
} | |||
|
|||
async function processFile(filename, localPath, comparePath) { | |||
const componentName = localPath.split("/")[localPath.split("/").length - 2]; |
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.
Unused variable
@@ -371,13 +363,34 @@ async function main( | |||
|
|||
cleanAndMkdir(pathing.latest); | |||
|
|||
const promises = []; |
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 moves the rendering assets to the .diff-output folder so it can be hosted together on Netlify.
@@ -480,11 +493,11 @@ async function main( | |||
|
|||
if (!hasAnyChange) { | |||
log.write(`\n${"✓".green} No changes detected.\n`); | |||
return Promise.resolve(); |
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.
Removing this lets the build continue with the web view whether changes are detected or not.
99448c8
to
9afcbe6
Compare
9afcbe6
to
dafd056
Compare
c68f4e3
to
d2272ea
Compare
92504d4
to
489c55a
Compare
e42e913
to
d1b088b
Compare
Co-authored-by: rise-erpelding <[email protected]> Co-authored-by: [ Cassondra ] <[email protected]>
d1b088b
to
87df661
Compare
Description
This cleans up a bit of the rendering for the compiled compare tool that lets us see any changes to the compiled output so that changes caused by updates to tooling do not get missed during review.
Why does this update remove the netlify.toml?
To build this view for PRs separately from our storybook deploy, we need to maintain different base directories and commands in netlify. This is done more easily in the Netlify UI than it is handled in our local *.toml asset. It also adds an additional layer of security to ensure updates made to these configs are handled by authenticated users.
How and where has this been tested?
Please tag yourself on the tests you've marked complete to confirm the tests have been run by someone other than the author.
Validation steps
yarn compare
Expect this command to build all CSS components and open an HTML preview listing file sizes with a badge identifying components with changes. Components will be listed on the side navigation for easier navigation through all the data.To-do list