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

HTML: Dynamic source snippets #15748

Merged

Conversation

pzuraq
Copy link
Contributor

@pzuraq pzuraq commented Aug 3, 2021

Issue: #12755

Adds dynamic source to the HTML 'framework' package.

What I did

Emits the rendered HTML if the HTML output was a string, otherwise does nothing. Output is also dedented by default. Mostly based on this PR: #15337

How to test

  • Is this testable with Jest or Chromatic screenshots?
  • Does this need a new example in the kitchen sink apps? -
  • Does this need an update to the documentation?

@nx-cloud
Copy link

nx-cloud bot commented Aug 3, 2021

Nx Cloud Report

CI ran the following commands for commit be1eea5. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

Status Command
#000000 nx run-many --target=prepare --all --parallel --max-parallel=15

Sent with 💌 from NxCloud.

@pzuraq
Copy link
Contributor Author

pzuraq commented Aug 3, 2021

One quick note, I couldn't seem to get this working locally in the kitchen sink examples. They just weren't showing code previews at all. Unsure if I did something wrong, could definitely use some pointers!

@shilman
Copy link
Member

shilman commented Aug 4, 2021

@pzuraq this is awesome!! 💯 working great for me:

html-dynamic-snippets

what I did:

yarn bootstrap --core
cd examples/html-kitchen-sink
yarn storybook

@yannbf
Copy link
Member

yannbf commented Aug 4, 2021

Hey @pzuraq this is an incredible contribution, thanks a lot for that! 🤩

I've got a couple of remarks:

1 - Could you check and update the snapshots in examples/html-kitchen-sink/tests/htmlshots.test.js -> Storyshots › Addons/Docs › standard source ? They were affected and the test is failing.

2 - Your commit doesn't link to your github user. Seems like your git config user.email is not correct, and as a result this contribution won't be linked to you. Is this something you expected? Otherwise you can change the author with something like:

git commit --amend --author="Your Name <your@email.com>"

The issues in the E2E tests don't seem to be related to your changes.

Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

Looking great @pzuraq -- love the tests 🚀

@shilman shilman self-assigned this Aug 4, 2021
Adds dynamic source to the HTML 'framework' package.
@pzuraq pzuraq force-pushed the add-dynamic-source-renderer-to-html branch from b8b55e9 to be1eea5 Compare August 4, 2021 13:35
@pzuraq
Copy link
Contributor Author

pzuraq commented Aug 4, 2021

@shilman ah, I didn't realize I had to run bootstrap again before using the kitchen sink apps! Cool, works perfectly now 😄

@yannbf thanks for pointing those out, I updated the snapshots, and the issue with the commit was my Git settings were off (changed companies recently, and started signing commits but forgot to upload my public PGP key 😱). Everything should be good to go now!

@shilman
Copy link
Member

shilman commented Aug 4, 2021

Great stuff @pzuraq 👏

@shilman shilman merged commit c80a50f into storybookjs:next Aug 4, 2021
@kerryj89
Copy link

kerryj89 commented Aug 4, 2021

Freakin' amazing! Thank you so much pzuraq!

@andrewgtibbetts
Copy link

andrewgtibbetts commented Aug 4, 2021

Thank you @pzuraq!!! I upgraded to pre-release but I am still getting "No code available" (in fact, the default install shows the same). Is there a setting to get this to work?

Storybook 6.4.0-alpha.24
Flavor: HTML

@andrewgtibbetts
Copy link

andrewgtibbetts commented Aug 4, 2021

If I change line 19 in the default install Button.stories.js from

  return createButton({ label, ...args });

to

 return createButton({ label, ...args }).outerHTML;

it works, since it returns a string instead of a DOM node / HTMLElement.
It doesn't seem to affect the rendering of the story, but might there be any consequences to doing this?

@maddesigns
Copy link

Awesome! 😍 Thanks @pzuraq! Is there a config to make it “prettier”?

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.

6 participants