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

update web-components to use variables from css-library #945

Merged
merged 8 commits into from
Nov 15, 2023

Conversation

it-harrison
Copy link
Contributor

@it-harrison it-harrison commented Nov 13, 2023

Chromatic

https://2236-remove-variables-file--60f9b557105290003b387cd5.chromatic.com


Description

This PR makes changes to allow for the removal of the variables.scss file from the web-component package, which is a duplicate of this file in css-library.
A companion PR for the css-library package must be merged and published before this PR is merged.

Closes 2236

Testing done

local testing done by using verdaccio to publish css-library and then consume in web-components:
css-library:

Screenshot 2023-11-13 at 2 00 13 PM

verdaccio:
Screenshot 2023-11-13 at 1 28 03 PM

web-components - build:

Screenshot 2023-11-13 at 1 48 29 PM

screenshots of some components that use variables from css-library

va-statement-of-truth
Screenshot 2023-11-13 at 2 12 12 PM

va-accordion
Screenshot 2023-11-13 at 2 10 30 PM

va-button
Screenshot 2023-11-13 at 2 14 07 PM

<va-segmented-progress-bar

Screenshot 2023-11-13 at 2 20 51 PM

Screenshots

Acceptance criteria

  • [ ]

Definition of done

  • Documentation has been updated, if applicable
  • A link has been provided to the originating GitHub issue (or connected to it via ZenHub)

@@ -27,6 +27,7 @@
"serve": "stencil build --dev --watch --serve"
},
"dependencies": {
"@department-of-veterans-affairs/css-library": "workspace:^",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

update the version here to be "1.0.0" when css-library published

@it-harrison it-harrison added the minor For a minor Semantic Version change label Nov 13, 2023
@it-harrison it-harrison marked this pull request as ready for review November 13, 2023 16:37
@it-harrison it-harrison requested a review from a team as a code owner November 13, 2023 16:37
@it-harrison it-harrison changed the title update repo to use variables from css-library update web-components to use variables from css-library Nov 13, 2023
Copy link
Contributor

@rmessina1010 rmessina1010 left a comment

Choose a reason for hiding this comment

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

LGTM

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.

@it-harrison - Thanks for working on this! Could you provide a little more context for how these changes were tested? Could you please also attach some screenshots to indicate visual parity was maintained when changing the variable import? Thank you!

Copy link
Contributor

@jamigibbs jamigibbs left a comment

Choose a reason for hiding this comment

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

@it-harrison Is it possible to clean up any lone-variables hanging around individual components like this now?

@it-harrison
Copy link
Contributor Author

@it-harrison - Thanks for working on this! Could you provide a little more context for how these changes were tested? Could you please also attach some screenshots to indicate visual parity was maintained when changing the variable import? Thank you!

@micahchiang ticket has been updated to show process

@it-harrison - Thanks for working on this! Could you provide a little more context for how these changes were tested? Could you please also attach some screenshots to indicate visual parity was maintained when changing the variable import? Thank you!

@micahchiang ticket has been updated with testing information / screenshots

@jamigibbs
Copy link
Contributor

@it-harrison I just wanted to make sure you saw my comment above. Is there a path forward yet for dealing with any existing variables in individual components? #945 (review)

@it-harrison it-harrison merged commit 4c9d250 into main Nov 15, 2023
6 checks passed
@it-harrison it-harrison deleted the 2236-remove-variables-file branch November 15, 2023 21:44
@it-harrison
Copy link
Contributor Author

@it-harrison Is it possible to clean up any lone-variables hanging around individual components like this now?

@jamigibbs we could move these variables into the css-library package? I'm not sure how else to leverage the fact that we are now installing css-library as a dependency.

@jamigibbs
Copy link
Contributor

jamigibbs commented Nov 16, 2023

@it-harrison Is it possible to clean up any lone-variables hanging around individual components like this now?

@jamigibbs we could move these variables into the css-library package? I'm not sure how else to leverage the fact that we are now installing css-library as a dependency.

That's probably the best approach. Can you write up a ticket for us to follow-up on doing that?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor For a minor Semantic Version change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Formation Deprecation - Step 1 - Move css variables from web-components/global to css library
5 participants