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

Core: Refactor preview rendering out of PreviewWeb #17598

Merged
merged 5 commits into from Mar 9, 2022
Merged

Conversation

tmeasday
Copy link
Member

@tmeasday tmeasday commented Mar 1, 2022

Precursor to #17214

What I did

Pulled the rendering part of PreviewWeb out into two classes: StoryRender and DocsRender.

  • StoryRender handles the job of "preparing" (ie. import-ing) the story
  • Then, depending on whether the mode the story is rendering (which needs to know if the story is docs-only, thus the preparing ahead of time), either renders or converts to a DocsRender
  • When modernInlineRender calls renderStoryToElement (one or more times on the docs page), this also creates additional StoryRenders.
  • The Preview simply "multiplexes" channel messages that drive re-renders to the StoryRenders and DocRender that are on the screen.

Some notes:

  • We don't yet handle the case of "tearing down" the render during preparing (PR to follow).
  • We only reload the screen when you tear down a story (change page) not when you reload the story (I think this is intended @ghengeveld?).

In any case this PR is not intended to change functionality, just refactor.

How to test

  • Is this testable with Jest or Chromatic screenshots?

See existing tests. I also added some for checking the re-rendering behaviour in modernInlineRender docs mode.

@tmeasday tmeasday added maintenance User-facing maintenance tasks core labels Mar 1, 2022
@nx-cloud
Copy link

nx-cloud bot commented Mar 1, 2022

☁️ Nx Cloud Report

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

lib/preview-web/src/PreviewWeb.tsx Outdated Show resolved Hide resolved
lib/preview-web/src/PreviewWeb.tsx Outdated Show resolved Hide resolved
lib/preview-web/src/PreviewWeb.tsx Outdated Show resolved Hide resolved
lib/preview-web/src/PreviewWeb.tsx Outdated Show resolved Hide resolved
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.

Can you document the control flow somewhere? It seems like there are a few different layers of indirection now and I'm having trouble following.


private canvasElement?: CanvasElement;

private context?: DocsContextProps;
Copy link
Member

Choose a reason for hiding this comment

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

does the type DocsContextProps make sense anymore? seems something like DocsContext might be more appropriate (tho that's probably taken)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I was just mainly moving stuff around but it did occur to me that name was a bit "unexpected".

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, it seems to be called that all over the place so I'm inclined to leave it in this refactor

@shilman shilman changed the title Refactor preview rendering out of PreviewWeb Core: Refactor preview rendering out of PreviewWeb Mar 9, 2022
@shilman shilman merged commit 70c0e96 into next Mar 9, 2022
@shilman shilman deleted the refactor-story-render branch March 9, 2022 07:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core maintenance User-facing maintenance tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants