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

[Bug] Button border and background color fixes for Secondary variants #36

Merged
merged 8 commits into from
Nov 22, 2023

Conversation

narin
Copy link
Contributor

@narin narin commented Nov 21, 2023

Ticket department-of-veterans-affairs/va-mobile-app#7330

Description of Change

  • Updates border color for Secondary variant in dark mode to #58b4ff (colorUswdsSystemColorBlueVivid30)
  • Updates background colors for Secondary and BaseSecondary variants to be transparent instead of black in both light and dark mode
  • Update link to correct documentation URL
  • Sort Storybook variants using underscore workaround

Testing Packages

Screenshots/Video

Screenshot 2023-11-21 at 9 58 50 AM

Testing

  • Tested on iOS
  • Tested on Android
  • Tested on Web

PR Checklist

Code reviewer validation:

  • General
    • PR is linked to ticket(s)
    • Acceptance criteria:
      • All satisfied or
      • Documented reason for not being performed or
      • Split to separate ticket and ticket is linked by relevant AC(s)
    • Above PR sections adequately filled out
    • If any breaking changes, in accordance with the pre-1.0.0 versioning guidelines: a CU ticket has been created for the VA Mobile App detailing necessary adjustments with the package version that will be published by this ticket
  • Code
    • Tests are included if appropriate (or split to separate ticket)
    • New functions have proper TSDoc annotations

Publish

If changes warrant a new version per the versioning guidelines and the PR is approved and ready to merge:

@narin narin marked this pull request as ready for review November 21, 2023 20:38
@narin narin requested a review from a team as a code owner November 21, 2023 20:38
@@ -72,17 +72,15 @@ export const Button: React.FC<ButtonProps> = ({
}
break
case ButtonVariants.BaseSecondary:
bgColor = DesignTokens.colorWhite
bgColorPressed = DesignTokens.colorWhite
bgColor = 'transparent'
Copy link
Contributor

@TimRoe TimRoe Nov 21, 2023

Choose a reason for hiding this comment

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

Should we have a 'transparent' token?

@jessicawoodin thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

In Figma, we wouldn't use a design token for this, so I'm leaning towards not creating one here either. In Figma, if we wanted to make something transparent, we just wouldn't apply a "fill color" to that layer which seems consistent with how it's being done here.

Copy link
Contributor

@TimRoe TimRoe left a comment

Choose a reason for hiding this comment

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

Approved. Did discover and created a bug poking at it in Web Storybook, but it appeared not directly related to this ticket (as it is also broken on the published Storybook).

@narin narin merged commit 9f1e943 into main Nov 22, 2023
6 of 7 checks passed
@narin narin deleted the bug/7330-narin-button-fixes branch November 22, 2023 19:04
@narin narin changed the title Bug/7330-narin-button-fixes [Bug] – Button border and background color fixes for Secondary variants Nov 27, 2023
@narin narin changed the title [Bug] – Button border and background color fixes for Secondary variants [Bug] Button border and background color fixes for Secondary variants Nov 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants