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

docs(components): Convert another bundle to Storybook #1045

Merged
merged 19 commits into from
Feb 23, 2023

Conversation

rodrigoeidelvein
Copy link
Contributor

Motivations

Convert the first bundle of components to Storybook.

Changes

Added

Storybook pages for:

  • InputText
  • Checkbox
  • DescriptionList
  • Content
  • FormField
  • LightBox
  • StatusLabel
  • Spinner

Changed

Deprecated

Removed

Fixed

Security

Testing


In Atlantis we use Github's built in pull request reviews.

Random photo of Atlantis

@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Feb 17, 2023

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 78068aa
Status: ✅  Deploy successful!
Preview URL: https://475ad933.atlantis.pages.dev
Branch Preview URL: https://job-62421-convert-mdx-bundle.atlantis.pages.dev

View logs

<Meta title="Components/LightBox" component={LightBox} />

# Light Box

Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to be missing the other styling when the lightbox is open
Storybook:
image

Docs currently:

image

Copy link
Contributor

Choose a reason for hiding this comment

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

Also doesn't show on the canvas tab

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 knew I'd have nightmares with this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This looks like an issue with the library react-image-lightbox, but it's already deprecated and it's been a while since the last update.

I tried upgrading it to 5.1.4 because they fixed this issue that is related to the lightbox being inside an iFrame (our case), but no success :(

Copy link
Contributor

Choose a reason for hiding this comment

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

Are we able to use the library that is recommended instead?
https://www.npmjs.com/package/react-photoswipe-gallery

We may want to add it to our list of packages to upgrade because it is no longer maintained.

I feel like this should be a blocker but I will defer to @chris-at-jobber

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah this is a pretty bad representation of the component and this will also impact viewers of Gallery as that component consumes Lightbox

Copy link
Contributor

Choose a reason for hiding this comment

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

If we can split it out to solve independently and land the rest of these docs, that could be OK for now. Would really like InputText to be available 😄

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 removed LightBox from this PR so we can land the other ones.

Copy link
Contributor

@MichaelParadis MichaelParadis left a comment

Choose a reason for hiding this comment

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

Minor issue with the light box story

Copy link
Contributor

@MichaelParadis MichaelParadis left a comment

Choose a reason for hiding this comment

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

Overall looks good. I did notice that if I opened a link in a new tab it wouldn't work.
The url would be something like http://localhost:6005/iframe.html?path=/docs/components-inlinelabel--inline-label which doesn't seem to be correct.
I know storybook has a link addon that may solve this.

Since we have the follow up subtask I don't think this is a blocker for this PR
https://storybook.js.org/addons/@storybook/addon-links

@jobbiebot jobbiebot bot added the approved label Feb 23, 2023
@MichaelParadis
Copy link
Contributor

It may also be related to the format of the links seen here: https://storybook.js.org/docs/react/writing-docs/mdx#linking-to-other-stories-and-pages

@rodrigoeidelvein
Copy link
Contributor Author

@MichaelParadis I'll handle the links in another PR. Thanks!

@rodrigoeidelvein rodrigoeidelvein merged commit 18bfec2 into master Feb 23, 2023
@rodrigoeidelvein rodrigoeidelvein deleted the JOB-62421-convert-mdx-bundle-5 branch February 23, 2023 16:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants