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

Don't change rootElement when received node is the same #4822

Merged
merged 1 commit into from
Nov 22, 2018

Conversation

goncalo-matos
Copy link
Contributor

Issue:
storybook-html's render is always rewriting the root element's content even when it's not a different component. This appeared to be a problem for dynamic class changes (as in changing a prop in a AngularJS component) as it is removing the dom node and adding it again which kills stuff like animations (since from the browser's perspective this is not a change in the class attribute and it is a new node).

Props to @svipatov for finding the issue and help developing this PR.

What I did

Checked the reference of the received DOM node, if it is the same then the node was already mutated and there is no need to rewrite the root element.

How to test

  • Is this testable with Jest or Chromatic screenshots?
    It could be, but I did not find any test for the changed file (I can add it if it's wanted)
  • Does this need a new example in the kitchen sink apps?
    No
  • Does this need an update to the documentation?
    No

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

@igor-dv igor-dv added the html label Nov 20, 2018
@igor-dv
Copy link
Member

igor-dv commented Nov 20, 2018

Can you maybe share a usecase ? I can think of knobs changes, but for this, we have a forceRender parameter that can be helpful. Here is the example in @storybook/react

@igor-dv igor-dv added the bug label Nov 20, 2018
@codecov
Copy link

codecov bot commented Nov 20, 2018

Codecov Report

Merging #4822 into next will decrease coverage by 0.05%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##             next    #4822      +/-   ##
==========================================
- Coverage   35.55%   35.49%   -0.06%     
==========================================
  Files         560      560              
  Lines        6827     6820       -7     
  Branches      915      914       -1     
==========================================
- Hits         2427     2421       -6     
  Misses       3920     3920              
+ Partials      480      479       -1
Impacted Files Coverage Δ
app/html/src/client/preview/render.js 0% <0%> (ø) ⬆️
app/react/src/server/cra-config.js 16.66% <0%> (-11.54%) ⬇️
app/react/src/server/framework-preset-cra-rules.js
...pp/react/src/server/framework-preset-cra-styles.js 0% <0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d507e2e...5a33ea0. Read the comment docs.

@svipatov
Copy link

Hey @igor-dv.

The case you refer is exactly in the case of using knobs.

When changing a knob, it causes the whole content of root to be replaced, so no transitions happen since there was no class "added".

I understand what you mean regarding forceRender.
But our problem is the inverse of what you've provided in the example regarding forceRender, since it looks like @storybook/html is "forcing" rendering everytime. This case is to help reuse the component, when a rerender is not needed.

Tell me if I've understood anything wrong and we'll try to elaborate more.

@igor-dv
Copy link
Member

igor-dv commented Nov 20, 2018

Maybe forceRender is a bit misleading name, but it means that the story was rerendered forcibly (and not navigated). In other words, when knobs are changed, this parameter will be true.

@goncalo-matos
Copy link
Contributor Author

Yeah, from your explanation it seems forceRender will do the trick. We'll change it and update/reopen the PR

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.

3 participants