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

M3-5611: Update Selection Card story #8206

Merged
merged 5 commits into from
Feb 17, 2022

Conversation

HanaXu
Copy link
Contributor

@HanaXu HanaXu commented Feb 16, 2022

Description

  • Clean up, add some documentation, and convert the Selection Card story to MDX format
  • Remove unused title prop from SelectionCard component

How to test

@HanaXu HanaXu self-assigned this Feb 16, 2022
Copy link
Contributor

@bnussman bnussman left a comment

Choose a reason for hiding this comment

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

Nice Job

One thing I'm seeing is that selecting Arch Linux or Debian results in no icon.

Screen Shot 2022-02-16 at 5 23 16 PM

Also, I see the filename is SelectionCardNew.stories.mdx. Could the word New be removed?

@HanaXu
Copy link
Contributor Author

HanaXu commented Feb 16, 2022

@bnussman I'm not sure why the image icons aren't working, they weren't working in the old tsx file either 🤔 .

Edit: Created M3-5730 to address this

Copy link
Contributor

@DevDW DevDW left a comment

Choose a reason for hiding this comment

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

From the controls, if you remove the subheadings, the story crashes:

Screen Shot 2022-02-17 at 12 24 33 PM

which is understandable since subheadings are required, but do we want to allow for this?

Also, if you attempt to add subheadings back after removing the array, Storybook defaults it to an object. Is there a way to have it default to an array?

@HanaXu
Copy link
Contributor Author

HanaXu commented Feb 17, 2022

@DevDW Looks like Storybook is using a library for the JSON tree view (storybookjs/storybook#12824). It doesn't look like the root was deletable in this comment, but they were pulling code into the Controls addon directly. So, I'm not sure if there's much we can do on our side.

Copy link
Contributor

@tiffwong tiffwong left a comment

Choose a reason for hiding this comment

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

Added one optional comment but everything else looks good!

@HanaXu HanaXu merged commit 22e223d into linode:develop Feb 17, 2022
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.

4 participants