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

UIP-2615 Redraw only once when store triggers along with ancestor rerender #103

Merged

Conversation

greglittlefield-wf
Copy link
Contributor

@greglittlefield-wf greglittlefield-wf commented Aug 8, 2017

Depends on Workiva/w_flux#96

Ultimate Problem

Flux components were rerendered excessively when nested inside each other and a store trigger caused rerendering of those nested components.

See reduced test case here.

Solution

Track whether components were already redrawn during the current redraw batch.

All consumers will need to do is make sure to call super.componentDidUpdate in overrides, and they'll get this fix.

Testing

  • Verify tests pass.
  • Verify issue is resolved in linked reduced test case.

Areas of Regression

Flux component rerendering


FYA @Workiva/web-platform-pp @Workiva/ui-platform-pp
FYI: @kylemcmorrow-wf @johnbland-wf @brentschuele-wf @brandonrodgers-wf @robbielamb-wf

@aviary2-wf
Copy link

aviary2-wf commented Aug 8, 2017

Raven

Number of Findings: 0

pubspec.yaml Outdated
w_flux:
git:
url: git@github.com:greglittlefield-wf/w_flux.git
ref: 189a1f97d95e0e3c446a5192c54e890fa61ebfb4 # from multiple_flux_redraws
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO bump min w_flux version once Depends on w_flux PR: Workiva/w_flux#96 is released

@johnbland-wf
Copy link

Great to see this. It has been rough on us.

FYI @kylemcmorrow-wf.

@rmconsole3-wf rmconsole3-wf changed the title Redraw only once when store triggers along with ancestor rerender UIP-2615 Redraw only once when store triggers along with ancestor rerender Aug 10, 2017
@jacehensley-wf
Copy link
Contributor

+1

@jacehensley-wf
Copy link
Contributor

@greglittlefield-wf Can you bump the w_flux dep since the change got released?

@greglittlefield-wf
Copy link
Contributor Author

@jacehensley-wf The w_flux PR was merged, but hasn't been released just yet.

@codecov-io
Copy link

Codecov Report

Merging #103 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #103      +/-   ##
==========================================
+ Coverage   94.36%   94.36%   +0.01%     
==========================================
  Files          31       31              
  Lines        1540     1541       +1     
==========================================
+ Hits         1453     1454       +1     
  Misses         87       87

@aaronlademann-wf
Copy link
Contributor

QA +10

  • Testing instruction
  • Dev +1's
  • Dev/QA +10
  • Unit tests created/updated
  • All unit tests pass
  • Rosie ran/Rosie comment displays expected info
  • Dependency Scan Clean

Merging.

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.

9 participants