Visitar URL original
USWDS - Button: Create story for links styled as buttons by mahoneycm · Pull Request #5435 · uswds/uswds · GitHub
Skip to content

USWDS - Button: Create story for links styled as buttons - #5435

Merged
thisisdano merged 19 commits into
developfrom
cm-button-link-story
Dec 6, 2023
Merged

thisisdano merged 19 commits into
developfrom
cm-button-link-story

Conversation

@mahoneycm

@mahoneycm mahoneycm commented Aug 14, 2023 •

Copy link
Copy Markdown
Contributor

Summary

Created a storybook preview for links styled as buttons. The story matches existing button storybook previews.

Breaking change

This is not a breaking change.

Related issue

Closes #5254

Related pull requests

Changelog →

This is an internal change.

Preview link

Links styled as buttons →

Problem statement

Currently the only links with the usa-button class in storybook are the "back" button on button-group & the card buttons (after #4385)

Solution

Create a storybook preview for links styled as buttons.

Notes:

  • Removed disabled button from link preview
    • Anchor elements cannot be disabled via the attribute.
    • Users must use our disabled classes + remove pointer events to disable links
  • Excluded type storybook control from Link story
    • I excluded the type attribute from the Links as Buttons template as well as the control. Anchor elements do not have type functionality.
  • aria_disabled control

Testing and review

  1. Inspect storybook preview
  2. Confirm controls work
  3. Confirm storybook includes all necessary elements to showcase links styled as buttons

Testing Checklist

  • Story appropriately showcases links styled as buttons
  • Story and controls are configurated as we would prefer
  • Test template matches existing patterns
  • Variants apply correctly

Further Considerations

Should we add links styled as buttons to the component preview?

Image

image

@mahoneycm
mahoneycm marked this pull request as ready for review August 14, 2023 16:21
@mahoneycm
mahoneycm requested review from amyleadem and mejiaj August 14, 2023 16:21

@amyleadem amyleadem left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for setting this up, @mahoneycm. It's looking good. I had just a couple thoughts:

  • It would be helpful to also have dropdown control that lets us include a button variant (like usa-button--accent-cool) in the modifiers.
  • I think we should clearly label this as a test pattern (unless we decide to consider it differently). I added a few notes below.

Comment thread packages/usa-button/src/usa-button.stories.js Outdated
Comment thread packages/usa-button/src/content/usa-button~link.json Outdated
Comment thread packages/usa-button/src/usa-button.stories.js Outdated
@mahoneycm

Copy link
Copy Markdown
Contributor Author

@amyleadem Updated the story to follow test patterns in 01641ba !

As for the dropdown, I had the same idea actually! I ended up going against it though because then you could only assign one modifier at a time. This would mean we couldn't do things like usa-button--outline usa-button--inverse or usa-button--accent-warm usa-button--big for example.

If we don't think that's necessary, I definitely think it could be a cool enhancement for the button story.

Honestly, maybe we could have a secondary dropdown for additional modifier classes?

@mahoneycm
mahoneycm requested a review from amyleadem September 21, 2023 20:11

@amyleadem amyleadem left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@mahoneycm That’s a good point about the modifier control. I think the text box control type works well given the circumstances.

Everything is looking good in Storybook here. However, I noticed that after moving the JSON to the test-patterns directory I now get an error when I run the build:html command. I recently encountered this issue on another PR as well. Here is the error:

[webpack-cli] Failed to load '/Users/aleadem/Dev/github/uswds-workspace/uswds/webpack.twig.config.js' config
[webpack-cli] Error: ENOENT: no such file or directory, open './packages/usa-button/src/test/test-patterns/usa-button.json'

From what I've seen, it appears to happen when the test-patterns directory contains both a twig and a JSON file. Since the JSON is so simple here, I am wondering if it makes sense to just pass the data as args in stories.js or if we want to dive deeper into what is happening in the webpack config.

@mahoneycm

Copy link
Copy Markdown
Contributor Author

@amyleadem Thanks for catching that! I wonder what the issue with having both file types is...

I went ahead and moved the data into args within stories.js for the time being. 👍

@mahoneycm
mahoneycm requested a review from amyleadem October 19, 2023 20:49

@amyleadem amyleadem left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looking good! Just a couple of small notes:

Comment thread packages/usa-button/src/usa-button.stories.js Outdated
Comment thread packages/usa-button/src/test/test-patterns/test-button-links.twig
@mahoneycm
mahoneycm requested a review from amyleadem November 16, 2023 17:23

@amyleadem amyleadem left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good to me!

@mejiaj mejiaj left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for addressing this issue.

Notes

Typically we use test stories to see a component within another context (like its behavior with other components or alongside other variants).

This is simply the same component, but in link form. It might make more sense to either:

  1. Make it a regular story variant.
  2. Make it a true test and add real buttons alongside.

Either could include a disclaimer that this is experimental or for prototyping.

modifier: "",
text: "Link styled as button",
};
TestLinksAsButtons.argTypes = {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Usually the name for tests should be just test and it tests more than just the component.

A description, if its a true test, as to what we're testing would be helpful.

Otherwise, it could be moved out of tests and alongside the other variants. If we don't have reasons to validate it as a "true" component, we can add a disclaimer.

image

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I updated it to follow standard component story patterns! Since it's used throughout our site and the design system, it deserves to be a real story with the other button variants!

@mahoneycm
mahoneycm requested a review from mejiaj November 27, 2023 17:38

@mejiaj mejiaj left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks better as a story, thank you. Can you look into the build errors before approving?

CircleCI build error

ERROR in Conflict: Multiple assets emit different content to the same filename usa-button.html

ERROR in Conflict: Multiple assets emit different content to the same filename usa-button~accent-cool.html

ERROR in Conflict: Multiple assets emit different content to the same filename usa-button~accent-warm.html

ERROR in Conflict: Multiple assets emit different content to the same filename usa-button~base.html

ERROR in Conflict: Multiple assets emit different content to the same filename usa-button~big.html

ERROR in Conflict: Multiple assets emit different content to the same filename usa-button~outline-inverse.html

ERROR in Conflict: Multiple assets emit different content to the same filename usa-button~outline.html

ERROR in Conflict: Multiple assets emit different content to the same filename usa-button~secondary.html

ERROR in Conflict: Multiple assets emit different content to the same filename usa-button~unstyled.html

webpack 5.76.0 compiled with 9 errors in 4524 ms

https://app.circleci.com/pipelines/github/uswds/uswds/5592/workflows/24c8302a-f207-4ec9-b0dd-f9d05ce20126/jobs/10050

@mahoneycm
mahoneycm requested a review from mejiaj November 29, 2023 21:41
@mahoneycm

Copy link
Copy Markdown
Contributor Author

@mejiaj build failure was a result of having multiple twig files with imported contend in the same directory it seems. Followed the pattern set by other components and moved the twig file into it's own directory and it resolved the issue! 👍

@mejiaj mejiaj left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, thanks for the adjustments

@mahoneycm
mahoneycm requested a review from thisisdano December 1, 2023 18:41

@thisisdano thisisdano left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Good conversation, good outcome

@thisisdano
thisisdano merged commit 769505a into develop Dec 6, 2023
@thisisdano
thisisdano deleted the cm-button-link-story branch December 6, 2023 17:19
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.

USWDS - Create story for links styled as buttons

4 participants