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

Replacing color-warning-message color name #978

Merged
merged 2 commits into from
Oct 20, 2023

Conversation

Andrew565
Copy link
Contributor

Description

Relates to #2047.
Changes the name of color-warning-message to color-feedback-warning-background.

Testing done

Tested locally; builds OK, and passes tests.

Screenshots

No visual changes.

Acceptance criteria

  • Name is changed

Definition of done

  • Changes have been tested in vets-website
  • Changes have been tested in IE11, if applicable
  • Documentation has been updated, if applicable
  • A link has been provided to the originating GitHub issue (or connected to it via ZenHub)
  • No sensitive information (i.e. PII/credentials/internal URLs/etc.) is captured in logging, hardcoded, or specs

New color name is color-feedback-warning-background
@Andrew565 Andrew565 self-assigned this Oct 20, 2023
@Andrew565 Andrew565 requested a review from a team as a code owner October 20, 2023 15:14
@@ -105,7 +105,7 @@ $color-primary-alt-darkest: #046b99;
$color-green-darker: #195c27;
$color-va-accent: #988530; // New gold "metal" acccent
$color-gold: #fdb81e;
$color-warning-message: #fac922; // Used for Disability benefits
Copy link
Contributor

Choose a reason for hiding this comment

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

This variable is being used in vets-website still. I'm wondering what would be the best way to do this changeover so they aren't interrupted.

Maybe we need to keep this variable here for now and add $color-feedback-warning-background too? Then update vets-website after it's available?

Screenshot 2023-10-20 at 10 15 54 AM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For what it's worth, I'm changing this in vets-website right now, too, so there won't be any interruption. That PR should be up in the next few minutes.

Copy link
Contributor

Choose a reason for hiding this comment

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

This repo is not directly linked to vets-website so just merging the PR won't make it available there. A new release of Formation will need to be created with this change and vets-website updated with the new version too (in your PR there would probably be easiest).

Before this PR is merged, you'll need to update the package.json version number and then follow step 3 in the docs for publishing Formation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, looks like you were right, I can't make the change in vets-website until after I've made that variable available in formation. So we either have to publish the new version of formation and then bring it in at the same time as the variable name change, or make both variables available in formation, bring it in and change vets-website, and then remove it from formation and publish another release. My preference would be to just publish a new formation release once, personally.

Copy link
Contributor

Choose a reason for hiding this comment

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

My preference would be to just publish a new formation release once

yeah, I agree. It would be a hassle to make more than one release.

publish the new version of formation and then bring it in at the same time as the variable name change

That feels like the most straightforward approach. 🚀

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Version bumped, I believe the PR needs to be merged into master before I can publish.

@Andrew565 Andrew565 merged commit 4767798 into master Oct 20, 2023
6 checks passed
@Andrew565 Andrew565 deleted the 2047-change-warning-color-name branch October 20, 2023 17:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants