[Feature] Component Implementation - Button

[narin]

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

Description of Change

• Implemented VAButton with the following variants
  • Base
  • BaseSecondary
  • Primary
  • Secondary
  • Destructive
  • White (this is a one-off variant used only on the VA Mobile App's login screen)
• Tokenized all colors used (aside from White variant onPress)
  • Note: Some tokens either had duplicates in VADS tokens OR were missing completely
  • For those that had duplicates between VADS's color.json and uswds.json, I used the VADS naming
  • Missing color tokens were found on USWDS's site
• Published v0.2.0 of tokens to NPM
• Added Storybook stories for each variant
• Added unit tests for rendering, onPress, and light mode variants

Notable differences from original VAButton

• Simplified to use Pressable's built in pressed state instead of keeping track of it in a useState hook
• Use enum for button variants instead of constants + type
• Removed props
  • hideBorder (this is instead built into the variant styling)
  • minHeight (not used anywhere in the app)
  • iconProps (not part of VADS)
  • disabled (VADS discourages disabled state)
  • disabledText (VADS discourages disabled state)
  • accessibilityState (not used and not needed without a disabled state)

Breaking changes

• Appointment details screen currently uses a disabled button which is not supported by our component library. We will need to work with health PM to create a ticket.

Testing Packages

• components-v0.1.1-alpha.0
• components-v0.1.1-alpha.1
• components-v0.1.1-alpha.2
• components-v0.1.1-alpha.3
• components-v0.1.1-alpha.4
• components-v0.1.1-alpha.5
• components-v0.1.1-alpha.6
• components-v0.1.1-alpha.7
• components-v0.1.1-alpha.8
• components-v0.1.1-alpha.9
• components-v0.1.1-alpha.10
• components-v0.1.1-alpha.11
• components-v0.1.1-alpha.12
• components-v0.1.1-alpha.13
• components-v0.1.1-alpha.14
• components-v0.1.1-alpha.15
• tokens-v0.1.13-alpha.0
• tokens-v0.1.13-alpha.1
• tokens-v0.1.13-alpha.2
• tokens-v0.1.13-alpha.3
• tokens-v0.2.1-alpha.1

Screenshots/Video

iOS

Simulator.Screen.Recording.-.iPhone.15.Pro.-.2023-11-08.at.10.57.06.mp4

Android

Screen.Recording.2023-11-08.at.11.04.02.AM.mov

Web

Screen.Recording.2023-11-08.at.11.20.11.AM.mov

White variant usage in mobile app

Simulator.Screen.Recording.-.iPhone.15.Pro.-.2023-11-16.at.12.00.08.mp4

Testing

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

PR Checklist

Code reviewer validation:

• General
  • PR is connected 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:

• Merge main into branch
• Publish new version
• Merge branch to main

[TimRoe]

Nitpicky so don't feel strongly, but if there's a simple way within the Doc section to bump Primary to be at the top instead of Base that would probably make more sense.

[narin]

Could not figure out a way to sort these properly. According to Storybook docs, they're supposed to be sorted by the way they are written in code but I'm not finding that to be the case. There's ways to set the manually, but we'd have to list out each story in an array which will be a bit cumbersome to do everytime we add a new story.

[TimRoe]

Yeah, initially tried just moving it to the top and was disappointed to see the sort didn't change. Oh well.

[TimRoe]

Update:
Had a fresh idea about how to fix webStorybookColorScheme issues. Followed it through, seemed good, and should fix the package in the app (didn't test to verify) since we now no longer import storybook-dark-mode directly at all and, correspondingly, I was able to move that package to a devDependency where it belongs so it won't be part of the published package.

Still didn't look at the updated unit tests yet, but everything else looks good (including testing iOS/Android/Web) except some minor comments.

[TimRoe]

Was testOnlyPressed also needed for detox or anything? If not, wonder if there's a way to check the pressed state without needing to add a prop to force it--seems like there should be. Taking a very brief glance at the testing library docs, I see fireEvent.keyDown exists and wonder if there's similarly a press-and-hold function that would also yield the pressed styles. Or maybe even that one works even though it's not a "key" for us.

[narin]

Was initially trying to use fireEvent(button, 'pressIn') which is the RN equivalent, but it was not working to render the pressed styles. testOnlyPressed was the only one I had success with, but it's not ideal since it has to be exposed as a prop.

[TimRoe]

Thought about mentioning it, but you'd already ran with integrating into jest tests which is fine for now.

However: wonder if we should explore other testing options (maybe jest has extensions for it too?) that do an actual UI snapshot to test styles instead of programmatically doing it which is a bit forced. I know at some RN virtual conference back in like 2020 or 2021 there was a presentation on a visual testing library that would just have pictures of before and compare to the after and highlight any differences down to the pixel (intentional or not) to confirm. Probably beyond the scope of this ticket anyway, but maybe worth a spike to explore since that seems a much more intuitive way to unit tests styles if it wasn't too much of a lift. In theory, it'd probably be much quicker/less tedious as well if we committed to style testing every component which likely does make sense for a design system.

[narin]

Created spike ticket to explore this: https://github.com/department-of-veterans-affairs/va-mobile-app/issues/7319

[TimRoe]

Dropping a re-request changes to denote the end of the 2nd pass. Not sure any of the new comments absolutely need changes, but I think some tweaks would be good. I think the only truly outstanding thing is confirming it works in the app now--which the commit I added should hopefully allow.

[narin]

Update: Had a fresh idea about how to fix webStorybookColorScheme issues. Followed it through, seemed good, and should fix the package in the app (didn't test to verify) since we now no longer import storybook-dark-mode directly at all and, correspondingly, I was able to move that package to a devDependency where it belongs so it won't be part of the published package.

Still didn't look at the updated unit tests yet, but everything else looks good (including testing iOS/Android/Web) except some minor comments.

App runs and I can import the button successfuly, but TypeScript is giving complaints about the web related code:

[1] node_modules/@department-of-veterans-affairs/mobile-component-library/src/utils/index.tsx(7,10): error TS2305: Module '"react"' has no exported member 'useSyncExternalStore'.
[1] node_modules/@department-of-veterans-affairs/mobile-component-library/src/utils/index.tsx(17,31): error TS7006: Parameter 'callback' implicitly has an 'any' type.
[1] node_modules/@department-of-veterans-affairs/mobile-component-library/src/utils/index.tsx(18,5): error TS2304: Cannot find name 'window'.
[1] node_modules/@department-of-veterans-affairs/mobile-component-library/src/utils/index.tsx(19,18): error TS2304: Cannot find name 'window'.
[1] node_modules/@department-of-veterans-affairs/mobile-component-library/src/utils/index.tsx(21,26): error TS2304: Cannot find name 'window'.
[1] node_modules/@department-of-veterans-affairs/mobile-component-library/src/utils/index.tsx(21,39): error TS2304: Cannot find name 'window'.

[TimRoe]

App runs and I can import the button successfuly, but TypeScript is giving complaints about the web related code:

Ugh! Added another commit to fix--adding listeners this way still seems to work with it abstracted out so was able to pull it out similarly to our Expo initialization which should remove those TS complaints because now the code won't exist in the NPM package.

I also changed from listening to the top body in favor of the parent body--because I think it would've been bugged when the iframe was within the doc site (since then it'd be listening to the doc site "body" in the HTML at the top level instead of the one "body" above it that the toggle would be switching the className between "light" and "dark". Fun that in normal circumstances there should only be a single "body" in a website, but within the doc site there's 3: the doc site's, storybook's, and the component's (or 3 layers I guess, more if you account for each iframe having 2 body's each).

[TimRoe]

Checking iOS I see my follow-up fix caused a require cycle. Should be fine, but will give us an annoying warning in iOS/Android.

I'd say we should move to attempt to implement ticket 7194 over resolving the require cycle by effectively making a duplicate structure for try-catching a different file than circling back to main.tsx.

[narin]

@TimRoe Tested within the app and errors are gone. Thanks!

Here is an example of the the button being used on the Login screen:

Simulator.Screen.Recording.-.iPhone.15.Pro.-.2023-11-16.at.12.00.08.mp4

Address all other feedback. Ready for another round of review.

[TimRoe]

Approved, good to go through the publish checklist creating components package v0.2.0 and merge.