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] Text component: Update stories #593

Merged
merged 4 commits into from
Nov 20, 2024
Merged

Conversation

narin
Copy link
Contributor

@narin narin commented Nov 18, 2024

Description of Change

Update Text component storybook stories:

  • Update doc url
  • Conditionally hide tone and size props when variant is 'display'
  • Hide 'error' tone option in Heading story.
    • Note: Storybook only allows for 1 condition per control at the root level. Since I set the tone to be conditionally hidden for the display variant, I could not add another condition to hide the error option for tone when the variant is heading. Because of this, the error tone is only hidden on the Heading story and will still be visible if switching variants on the Docs page or other stories.

Also updates a missed requirement: bottomSpacing should only be fully customizable for Heading and Display variants. For Body, bottomSpacing should only be able to set to 'none' or left undefined to use the default.

Testing Packages

N/A

Screenshots/Video

Screen.Recording.2024-11-18.at.11.07.45.AM.mov

Testing

  • 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:

@narin narin changed the title Add docUrl. Disable tone and size for display variant. Limit tone for… [Feature] Text component: Update stories Nov 18, 2024
@narin narin marked this pull request as ready for review November 19, 2024 19:37
@narin narin requested a review from a team as a code owner November 19, 2024 19:37
@@ -52,6 +56,9 @@ export const Body: Story = {
const children = 'Lorem ipsum dolor sit amet.'

export const _Heading: Story = {
argTypes: {
tone: { control: 'radio', options: ['default', 'subtle', 'inverse'] },
Copy link
Contributor

Choose a reason for hiding this comment

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

Be a slight improvement if the options could be pulled directly from HeaderProps.tone or BaseTones so if any are ever added/removed it doesn't break the story.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TypeScript doesn't allow you to build an array from a type, but you can do the opposite. Declared and exported an array called baseToneValues in Text.tsx and updated the type to use that. The syntax is a little weird but solves the problem of hardcoding the options.

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.

Couple minor suggestions, but overall seems good to go.

@narin narin merged commit 74fb2a8 into main Nov 20, 2024
4 of 5 checks passed
@narin narin deleted the feature/570-narin-text-stories branch November 20, 2024 20:35
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