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

change docs to render stories out of context #14911

Closed
wants to merge 17 commits into from

Conversation

ndelangen
Copy link
Member

@ndelangen ndelangen commented May 13, 2021

Issue: #13145 #13277 #10817 #8601 #12726 #14447 #14333 #13074

What I did

I've overhauled the way we render stories inline.

I've removed the need for prepareForInline:
When docs encounters a story it needs to render inline, it creates div element with a unique id on it.
Then it emits an event triggering the framework's renderer to render a story unto the targetted div.

How to test

I'm likely missing some features, this is a POC / draft after all!

  • does args and controls still work
  • viewports
  • toolbar addons
  • HMR
  • switching back and forth between canvas & docs
  • does it work when docsOnly
  • performance

Known issues:

  • I'm not destroying / unmounting inline stories anymore.
    I do not know how bad this is, I assume memory leaks
  • Preact seems to have issues with actions addon, might be pre-existing
  • Vue2 doesn't re-render stories in docs-mode when controls / args changes
  • Angular doesn't work at all

ndelangen added 3 commits May 13, 2021 15:03
fire an async event to render into placeholder
removes need for prepareForInline and brings inline rendering of docs to all frameworks
@ndelangen ndelangen requested review from shilman and gaetanmaisse May 13, 2021 14:47
@ndelangen ndelangen added this to the 7.0 milestone May 13, 2021
@ndelangen ndelangen added addon: docs BREAKING CHANGE maintenance User-facing maintenance tasks labels May 13, 2021
@nx-cloud
Copy link

nx-cloud bot commented May 13, 2021

Nx Cloud Report

CI ran the following commands for commit cdf70bd. 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.

@github-actions
Copy link
Contributor

github-actions bot commented May 13, 2021

Fails
🚫

Please choose only one of these labels: ["BREAKING CHANGE","maintenance"]

🚫 PR is marked with "BREAKING CHANGE" label.

Generated by 🚫 dangerJS against cdf70bd

@ndelangen
Copy link
Member Author

This does require a change in all framework's renderers. so the ones we just extracted out (@shilman) might need a patch, if we decide that's worth it.

@shilman
Copy link
Member

shilman commented May 13, 2021

🤯 this is awesome @ndelangen 🤯

@ndelangen
Copy link
Member Author

@tooppaaa @gaetanmaisse @ThibaudAV
Are you able to assist with making the required changes to angular's code in render?

@ThibaudAV
Copy link
Contributor

When there is a navigation between two stroy of the same "stories" on the docs page all stories is loaded twice. I think it's a bug. You can see it with the logs. Do you think it is related to these changes?

const storyFn = () => unboundStoryFn(storyContext);

if (targetDOMNode.querySelector('[data-is-loadering-indicator="true"')) {
targetDOMNode.querySelector('[data-is-loadering-indicator="true"').remove();
Copy link
Contributor

Choose a reason for hiding this comment

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

for angular it forces the full rendering every time. 🤔
which differs from the canvas rendering which is optimized when only the args change

@ndelangen
Copy link
Member Author

@ThibaudAV What can we do with this PR versus this PR: #15002 and what has recently been happening on next?

@ThibaudAV
Copy link
Contributor

if this is still intended to be taken for the next version. once rebase I would do the same for the one with angular (which will be greatly reduced ^^) and I would test it still works properly
what do you think?

@ndelangen
Copy link
Member Author

When we have a solution that works across the board we can assess if this is truly a breaking change or not. If it is, it's a 7.0 feature.

We're not targeting 6.3. rather 6.4 or 7.0.
I'm not sure there will be a 6.4.

@shilman
Copy link
Member

shilman commented Jun 17, 2021

There will be a 6.4

@ThibaudAV
Copy link
Contributor

as you like ;) I'm pretty confident that it will work for angular with this PR

@amartinez62
Copy link

Double check for feature flags if they aren't there then we need to add them and it should be off by default. Double check this works with Angular.

@shilman
Copy link
Member

shilman commented Jul 12, 2021

I genericized feature flagging here: #15375

@shilman shilman modified the milestones: 7.0, 6.4 PRs Jul 22, 2021
tmeasday added a commit that referenced this pull request Aug 6, 2021
@agrohs
Copy link

agrohs commented Nov 3, 2021

What is the latest on this PR?

@ndelangen
Copy link
Member Author

I'm pretty sure this got merged via another PR somewhere in @tmeasday's work, can we close this @shilman ?

@shilman
Copy link
Member

shilman commented Nov 22, 2021

Thanks @ndelangen, closing this!

@shilman shilman closed this Nov 22, 2021
@stof stof deleted the tech/out-of-context-docs-rendering-stories branch May 25, 2022 09:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants