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

[Feature] Standalone Spacer component #322

Merged
merged 6 commits into from
May 15, 2024

Conversation

TimRoe
Copy link
Contributor

@TimRoe TimRoe commented May 14, 2024

Description of Change

  • Moved the Spacer component from utils to own folder/file.
  • Created unit tests validating default/custom sizing for both vertical and horizontal spacing
  • Created stories to demo the spacer functionality in Horizontal/Vertical capacity
    • Horizontal demoed similar to Link
    • Vertical demoed similar to Alert

Screenshots/Video

Horizontal (default size):
image

Vertical (default size):
image

Testing

Validated Spacer stories in iOS/Android/Web. The stories are a little janky on iOS/Android (Horizontal can go off the screen if you expand it, Vertical the Buttons have different widths due to not being in a container to span), but it functionally demonstrates the Spacer behavior given it being tricky to show a non-visible element. Validated unit tests fail as expected when changing the expect values.

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

PR Checklist

Code reviewer validation:

  • General
    • PR is linked to ticket(s)
    • PR has changelog label applied if it's to be included in the changelog
    • 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:

@TimRoe TimRoe changed the title [Feature] Split out Spacer component [Feature] Standalone Spacer component May 14, 2024
@TimRoe TimRoe marked this pull request as ready for review May 14, 2024 19:52
@TimRoe TimRoe requested a review from a team as a code owner May 14, 2024 19:52
Copy link
Contributor

@narin narin left a comment

Choose a reason for hiding this comment

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

Looks good. Regarding the weirdness in iOS/Android Storybook, do you think this could be resolved by adding some sort of wrapper with padding around our stories? I think we discussed this as well during the QA call with Adam and Tom.

@TimRoe
Copy link
Contributor Author

TimRoe commented May 15, 2024

Regarding the weirdness in iOS/Android Storybook, do you think this could be resolved by adding some sort of wrapper with padding around our stories? I think we discussed this as well during the QA call with Adam and Tom.

I think the weirdness with the Vertical story probably could, I assume it's fitting to the text length due to whatever I had wrapping it instead of spanning the full screen width like the Button otherwise does.

The Horizontal story is weird because I'm not sure RN does horizontal screen scrolling--it definitely wouldn't make sense to wrap though given the entire point of the spacer is to provide space within what's visible. The weirdness there is making it large makes the visual elements go off the screen, which is exactly what one would expect so it's not really broken at all per se, but just a little goofy.

@TimRoe TimRoe merged commit 3c195a9 into main May 15, 2024
12 of 17 checks passed
@TimRoe TimRoe deleted the feature/257-roettger-SpacerComponent branch May 15, 2024 19:15
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.

2 participants