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

Sweeping performance improvements and onload resizing #27

Merged
merged 2 commits into from Jan 5, 2018
Merged

Sweeping performance improvements and onload resizing #27

merged 2 commits into from Jan 5, 2018

Conversation

jeremyong
Copy link
Contributor

The pull request (in two separate commits) does the following:

Commit 1 (correctness pass)

Update elements in response to onload in addition to on resize to prevent stale sizing if layout is changed as a result of a script or element being loaded downstream. (authorship Kip Ricker, a coworker of mine)

Commit 2 (performance pass)

  • Elide unnecessary findDOMNode which is unnecessary since you already grab the element reference and this function performs poorly with other React replacements that manipulate the DOM more directly instead of indirect id lookups a la React.
  • Uses a single event listener for resize and onload events instead of one per component
  • Remove the read/write update cycle that occurs in the onBodyResize_ callback. The original code required each text element update to force a relayout of the page. What is done instead is all element widths are read once in a batch and font stylings are applied after in a batch.
  • Defer layout to the first animation frame after the initial mount. This will give the element layout a better chance of "settling" before all the font sizes get calculated on the initial run

Benchmark:

On a page with ~200 text elements, the render time went from 6 seconds on an iphone 7 to 30 ms. From a big-O standpoint, the algorithmic complexity is now O(n) compared to O(n^3)

@gianu
Copy link
Owner

gianu commented Jan 5, 2018

Wow, Awesome! Thanks for this PR! merging it.

@gianu gianu merged commit 79c494f into gianu:master Jan 5, 2018
@jeremyong
Copy link
Contributor Author

thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants