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.

Sign up for GitHub

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

Jump to bottom

Fix lib preview web story change event #18400

Closed
wants to merge 2 commits into from
Closed

Fix lib preview web story change event #18400

wants to merge 2 commits into from

Conversation

kroeder
Copy link
Member

@kroeder kroeder commented Jun 3, 2022

Issue: #18176 #18175

What I did

app/angular listens to the STORY_CHANGED in the DocsRenderer and this event has fired even if you are in the same docs tab and switch between stories of the same docs page.

This broke the whole docs tab because app/angular needs to reset the underlying Angular instance but could not recover from that inside the docs page. (see #18176 for more details)

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?

If your answer is yes to any of these, please make sure to include it in your PR.

Todos

  • Add a test

kroeder added 2 commits June 3, 2022 13:24
@kroeder
refactor(app/angular): remove DOCS_RENDERED listener
8074a21 
This is an unnecessary event listener since we
already listen to STORY_CHANGED and therefore
call `resetPlatformBrowserDynamic` twice for
no reason.
@kroeder
fix(lib/preview-web): trigger STORY_CHANGED only if componentId changes
df1ac94 
Previously, the STORY_CHANGED event got fired even if you are
inside a docs tab, seeing all stories. This has lead to a
bug in app/angular that lead to unmounting all stories from
the DOM without recovering.
@nx-cloud
Copy link

nx-cloud bot commented Jun 3, 2022 •
edited
Loading

☁️ Nx Cloud Report

CI is running/has finished running commands for commit df1ac94. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

@ndelangen ndelangen requested a review from tmeasday June 3, 2022 13:25
@ndelangen ndelangen added bug angular patch:yes Bugfix & documentation PR that need to be picked to main branch labels Jun 3, 2022
@tmeasday
Copy link
Member

tmeasday commented Jun 4, 2022

@kroeder I'd be pretty reticent to make this change for a couple of reasons:

  1. The STORY_CHANGED event is used by a lot of addons so we would want to be careful about changing its semantics without thinking it through carefully.

  2. I would be open to not even emitting STORY_CHANGED when docs is rendered but it is probably best thought of more holistically alongside other more drastic changes to docs coming in 7.0. cc @shilman who maybe has links to more details (?)

  3. I don't think it is safe to assume just because the component hasn't changed the docs page hasn't. It is possible to specify a completely different docs page for each story in a component. Again, I think we could think about the events that are emitted as stories and docs change and how that interacts with view layer rendering more carefully, but we should do it with future changes in mind.

@tmeasday
Copy link
Member

tmeasday commented Jun 4, 2022

Maybe in the meantime as a 6.x non breaking change this logic could be implemented on the angular side somehow?

@ndelangen ndelangen closed this Jun 9, 2022
@stof stof deleted the fix-lib-preview-web-story-change-event branch August 31, 2022 13:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tmeasday tmeasday Awaiting requested review from tmeasday

Assignees

@kroeder kroeder

Labels
angular bug patch:yes Bugfix & documentation PR that need to be picked to main branch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kroeder @tmeasday @ndelangen