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

Jump to bottom

3640 - update asset paths to reference css-library and uswds packages, add missing files to css-library #1519

Merged
merged 4 commits into from
Mar 11, 2025
Merged

3640 - update asset paths to reference css-library and uswds packages, add missing files to css-library #1519

merged 4 commits into from
Mar 11, 2025

Conversation

powellkerry
Copy link
Contributor

@powellkerry powellkerry commented Mar 10, 2025 •
edited
Loading

Chromatic

https://3640-css-library-assets--65a6e2ed2314f7b8f98609d8.chromatic.com

Description

  • Update assets paths for fonts and images to use the fully qualified package url.
  • v1 alert styles are generated from the uswds package, these urls needed to be programmatically updated during the build process. We should determine if these styles are still used and if not remove them.
  • Added missing files from formation and the USWDS package.

Closes 3640

QA Checklist

  • [ x] Component maintains 1:1 parity with design mocks
  • [ x] Text is consistent with what's been provided in the mocks
  • [ x] Component behaves as expected across breakpoints
  • [ x] Accessibility expert has signed off on code changes (if applicable. If not applicable provide reason why)
  • [ x] Designer has signed off on changes (if applicable. If not applicable provide reason why)
  • [ x] Tab order and focus state work as expected
  • [ x] Changes have been tested against screen readers (if applicable. If not applicable provide reason why)
  • New components are covered by e2e tests; updates to existing components are covered by existing test suite
  • Changes have been tested in vets-website using Verdaccio (if applicable. If not applicable provide reason why)

Screenshots

No discernable change -

va.gov original:
Screenshot 2025-03-10 at 1 35 52 PM

va.gov updated:
Screenshot 2025-03-10 at 1 36 38 PM

Acceptance criteria

  • QA checklist has been completed
  • Screenshots have been attached that cover desktop and mobile screens

Definition of done

  • Documentation has been updated, if applicable
  • [ x] A link has been provided to the originating GitHub issue

@powellkerry powellkerry requested a review from micahchiang March 10, 2025 16:17
@powellkerry powellkerry requested a review from a team as a code owner March 10, 2025 16:17
@powellkerry powellkerry added the major Major change in semantic versioning label Mar 10, 2025
powellkerry
powellkerry commented Mar 10, 2025
View reviewed changes
packages/css-library/package.json Outdated
"clean-dist": "rimraf dist/*",
"copy:scss-files": "cp -rv src/stylesheets/_override-function.scss dist/stylesheets/_override-function.scss && cp -rv src/stylesheets/_mixins.scss dist/stylesheets/_mixins.scss && cp -rv src/stylesheets/_functions.scss dist/stylesheets/_functions.scss && cp -rv src/stylesheets/formation-overrides/_variables.scss dist/stylesheets/formation-overrides/_variables.scss",
"copy-assets": "cp -rv src/assets/fonts dist/fonts/ && cp -rv src/assets/img dist/img/",
"copy": "node ./copy-uswds-color-tokens.js && yarn run copy-assets"
"copy": "node ./copy-uswds-color-tokens.js && yarn run copy-assets",
"clean-css-urls": "sed -i '' -e 's|\"/img/info.png\"|~@department-of-veterans-affairs/css-library/dist/img/info.png|g' ./dist/stylesheets/{,**/}*.css && sed -i '' -e 's|\"/img/info.svg\"|~@department-of-veterans-affairs/css-library/dist/img/info.svg|g' ./dist/stylesheets/{,**/}*.css && sed -i '' -e 's|\"/img/success.png\"|~@department-of-veterans-affairs/css-library/dist/img/success.png|g' ./dist/stylesheets/{,**/}*.css && sed -i '' -e 's|\"/img/success.svg\"|~@department-of-veterans-affairs/css-library/dist/img/success.svg|g' ./dist/stylesheets/{,**/}*.css && sed -i '' -e 's|\"/img/warning.png\"|~@department-of-veterans-affairs/css-library/dist/img/warning.png|g' ./dist/stylesheets/{,**/}*.css && sed -i '' -e 's|\"/img/warning.svg\"|~@department-of-veterans-affairs/css-library/dist/img/warning.svg|g' ./dist/stylesheets/{,**/}*.css && sed -i '' -e 's|\"/img/error.png\"|~@department-of-veterans-affairs/css-library/dist/img/error.png|g' ./dist/stylesheets/{,**/}*.css && sed -i '' -e 's|\"/img/error.svg\"|~@department-of-veterans-affairs/css-library/dist/img/error.svg|g' ./dist/stylesheets/{,**/}*.css"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is very ugly, but the v1 version of alert styles is being generated from the old uswds package so there is nowhere in our code that we can update these references until after build. We need to find out if these alert styles can get removed so that we don't need to do this :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd have to check to verify, but I'm fairly certain v1 alert styles aren't being used anywhere at this point 🤔

micahchiang
micahchiang reviewed Mar 10, 2025
View reviewed changes
Copy link
Contributor

@micahchiang micahchiang left a comment

Choose a reason for hiding this comment

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

Do we need all of these new images?

packages/css-library/package.json Outdated
"clean-dist": "rimraf dist/*",
"copy:scss-files": "cp -rv src/stylesheets/_override-function.scss dist/stylesheets/_override-function.scss && cp -rv src/stylesheets/_mixins.scss dist/stylesheets/_mixins.scss && cp -rv src/stylesheets/_functions.scss dist/stylesheets/_functions.scss && cp -rv src/stylesheets/formation-overrides/_variables.scss dist/stylesheets/formation-overrides/_variables.scss",
"copy-assets": "cp -rv src/assets/fonts dist/fonts/ && cp -rv src/assets/img dist/img/",
"copy": "node ./copy-uswds-color-tokens.js && yarn run copy-assets"
"copy": "node ./copy-uswds-color-tokens.js && yarn run copy-assets",
"clean-css-urls": "sed -i '' -e 's|\"/img/info.png\"|~@department-of-veterans-affairs/css-library/dist/img/info.png|g' ./dist/stylesheets/{,**/}*.css && sed -i '' -e 's|\"/img/info.svg\"|~@department-of-veterans-affairs/css-library/dist/img/info.svg|g' ./dist/stylesheets/{,**/}*.css && sed -i '' -e 's|\"/img/success.png\"|~@department-of-veterans-affairs/css-library/dist/img/success.png|g' ./dist/stylesheets/{,**/}*.css && sed -i '' -e 's|\"/img/success.svg\"|~@department-of-veterans-affairs/css-library/dist/img/success.svg|g' ./dist/stylesheets/{,**/}*.css && sed -i '' -e 's|\"/img/warning.png\"|~@department-of-veterans-affairs/css-library/dist/img/warning.png|g' ./dist/stylesheets/{,**/}*.css && sed -i '' -e 's|\"/img/warning.svg\"|~@department-of-veterans-affairs/css-library/dist/img/warning.svg|g' ./dist/stylesheets/{,**/}*.css && sed -i '' -e 's|\"/img/error.png\"|~@department-of-veterans-affairs/css-library/dist/img/error.png|g' ./dist/stylesheets/{,**/}*.css && sed -i '' -e 's|\"/img/error.svg\"|~@department-of-veterans-affairs/css-library/dist/img/error.svg|g' ./dist/stylesheets/{,**/}*.css"
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd have to check to verify, but I'm fairly certain v1 alert styles aren't being used anywhere at this point 🤔

micahchiang
micahchiang reviewed Mar 10, 2025
View reviewed changes
packages/css-library/src/stylesheets/base/va.scss
@@ -386,7 +386,7 @@ article > h1 {
padding: scale-rule(1rem 0);
}

background: url('#{$image-path}/stars.png') no-repeat center;
background: url(~@department-of-veterans-affairs/css-library/dist/img/stars.png) no-repeat center;
Copy link
Contributor

Choose a reason for hiding this comment

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

ah nice, cool to see that this works. I wasn't sure if we would need to rewrite relative paths or not. Hopefully we can just get away with referencing the node nodules path this way 🤞

@powellkerry
Copy link
Contributor Author

I found a few icons that were not being used, and also found a solution to the generated url issue.

@jamigibbs jamigibbs added css-library and removed major Major change in semantic versioning labels Mar 10, 2025
@powellkerry powellkerry merged commit 1c6bbba into main Mar 11, 2025
8 checks passed
@powellkerry powellkerry deleted the 3640-css-library-assets branch March 11, 2025 16:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@micahchiang micahchiang micahchiang approved these changes

Assignees
No one assigned
Labels
css-library
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

core.css assets are not properly bundled with css-library package as released on NPM
3 participants
@powellkerry @micahchiang @jamigibbs