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

refactor: [M3-7542] - TagsInput & TagsPanel Storybook v7 Stories #9963

Merged
merged 8 commits into from
Dec 12, 2023

Conversation

abailly-akamai
Copy link
Contributor

@abailly-akamai abailly-akamai commented Dec 5, 2023

Description 📝

Small PR to handle .tsx v7 stories for TagsInput & TagsPanel

There is no change that should affect client-side code. This only touches our book.

Changes 🔄

  • New v7 stories for TagsInput & TagsPanel
  • New test for TagsPanel
  • Move styles in own file (TagsPanel)
  • Remove stories list within documentation

Preview 📷

Screenshot 2023-12-06 at 11 42 58

Screenshot 2023-12-06 at 11 42 58

How to test 🧪

Verification steps

As an Author I have considered 🤔

Check all that apply

  • 👀 Doing a self review
  • ❔ Our contribution guidelines
  • 🤏 Splitting feature into small PRs
  • ➕ Adding a changeset
  • 🧪 Providing/Improving test coverage
  • 🔐 Removing all sensitive information from the code and PR description
  • 🚩 Using a feature flag to protect the release
  • 👣 Providing comprehensive reproduction steps
  • 📑 Providing or updating our documentation
  • 🕛 Scheduling a pair reviewing session
  • 📱 Providing mobile support
  • ♿ Providing accessibility support

@@ -89,7 +89,6 @@ const preview: Preview = {
<Description />
<Primary />
<Controls />
<Stories />
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Listing all stories in documentation often leads to state issues and it will make things a bit easier to test visual regression by focusing on individual stories.

Documentation is meant to be a playground on the default rendering of the component.

We can always put it back in if people feel it's necessary.

Copy link
Contributor

@coliu-akamai coliu-akamai Dec 7, 2023

Choose a reason for hiding this comment

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

I'm actually a bit confused where this is referring to 😅 - is this regarding the Documentation section of each story, like here:
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's to show the list of stories in the Documentation page under the args table

@abailly-akamai abailly-akamai marked this pull request as ready for review December 6, 2023 16:49
@abailly-akamai abailly-akamai requested a review from a team as a code owner December 6, 2023 16:49
@abailly-akamai abailly-akamai requested review from jdamore-linode, dwiley-akamai and coliu-akamai and removed request for a team December 6, 2023 16:49
Copy link

github-actions bot commented Dec 6, 2023

Coverage Report:
Base Coverage: 77.16%
Current Coverage: 78.35%

Copy link
Contributor

@coliu-akamai coliu-akamai left a comment

Choose a reason for hiding this comment

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

✅ tests
✅ storybook works!

Thanks Alban!

@@ -89,7 +89,6 @@ const preview: Preview = {
<Description />
<Primary />
<Controls />
<Stories />
Copy link
Contributor

@coliu-akamai coliu-akamai Dec 7, 2023

Choose a reason for hiding this comment

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

I'm actually a bit confused where this is referring to 😅 - is this regarding the Documentation section of each story, like here:
image

@coliu-akamai coliu-akamai added the Add'tl Approval Needed Waiting on another approval! label Dec 7, 2023
Copy link
Contributor

@dwiley-akamai dwiley-akamai left a comment

Choose a reason for hiding this comment

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

TagsPanel.test.tsx passes ✅


return (
<Box sx={{ height: 300 }}>
<TagsPanel {...args} tags={tags} updateTags={handleUpdateTags} />
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we have better support for disabled and align?

Currently when it's disabled, we lose the click-X-to-remove functionality for tags, but new tags can still be added (this isn't possible in prod Storybook).

Changing the selection for align doesn't seem to have any effect at all (this is true in prod Storybook also).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently when it's disabled, we lose the click-X-to-remove functionality for tags, but new tags can still be added (this isn't possible in prod Storybook).

I wasn't able to reproduce this but I remove align cause it's used nowhere and not even forwarded

disabled: false,
hideLabel: false,
label: '',
menuPlacement: 'bottom',
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we have better support for menuPlacement? Toggling between "top" and "bottom" has no effect (it's broken in prod Storybook also)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also wasn't able to reproduce this - menu placement gets toggled as expected. I did improve the styling of the story so the select sits in the middle of the canvas to help viewing it better tho.

Copy link
Member

@bnussman-akamai bnussman-akamai left a comment

Choose a reason for hiding this comment

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

Should we move the Tag story out from the Chip section to the Tag section?

Screenshot 2023-12-12 at 10 21 19 AM


const queryClient = new QueryClient();

const renderWithQueryClient = (ui: any) => {
Copy link
Member

Choose a reason for hiding this comment

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

Can we type this? Not sure if JSX.Element is the best or not

Also, should we move this to packages/manager/src/utilities/testHelpers.tsx?

Suggested change
const renderWithQueryClient = (ui: any) => {
const renderWithQueryClient = (ui: JSX.Element) => {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah yes i left this one out. sure thing - we can be more specific too using

React.ReactElement<TagsPanelProps>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also moved Tag there - makes sense!

@bnussman-akamai bnussman-akamai added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! labels Dec 12, 2023
@abailly-akamai abailly-akamai merged commit 68231ad into linode:develop Dec 12, 2023
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approved Multiple approvals and ready to merge! Storybook
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants