Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Faster UI - React Tweaks #3555

Merged
merged 4 commits into from
Nov 22, 2016
Merged

Faster UI - React Tweaks #3555

merged 4 commits into from
Nov 22, 2016

Conversation

ngotchac
Copy link
Contributor

Related to #3240

Mainly pre-render accounts list so the click on the Accounts tab instantaneously render something instead of waiting for all the accounts to be rendered.

Smarter Account Summary Component and better tabs (used to be rendered around 30 times whenever the tabs switched).

Also added React Perf in dev mode : in JS console:

  • Perf.start()
  • Perf.stop()
  • Perf.printWasted()

This will show in a table all the useless re-renders.

@ngotchac ngotchac added A0-pleasereview 🤓 Pull request needs code review. M7-ui labels Nov 21, 2016
static propTypes = {
style: PropTypes.object.isRequired,
Copy link
Contributor

Choose a reason for hiding this comment

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

Care to explain why we are passing this in while it is available on context?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just to not re-compute the background from the given seed for each render (it's computed once, at least while the seed isn't changing, by the Redux mapStateToProps method.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Couldn't it just have been done in componentWillMount?

I think the single calculation is great, just concerned about "special cases" where we start doing things inconsistently.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doing it in componentWillMountwould mean that it won't update on seed update. This could be done however in componentWillReceiveProps but it means it's linked to the state.
So we'd have to choose whether to do it in the component or have a state-less component with props managed by Redux. Not sure which is the best practice. Performance wise, it's kind of the same (if done correctly, ie. implementing shouldComponentUpdate if not state-less)

Copy link
Contributor

Choose a reason for hiding this comment

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

So we'd have to choose whether to do it in the component or have a state-less component with props managed by Redux.

A general note:

Both Redux & Mobx encourage users to split React components into "dumb"/presentational and "smart"/behavioural. We'd have a smart component connecting the store/state to the React components tree and lots of "dumb", pure components beneath.

However, memoizing the background computation and returning a value that can be checked for equality by React should work in terms of performance.

@jacogr jacogr added A8-looksgood 🦄 Pull request is reviewed well. A7-looksgoodtestsfail 🤖 Pull request is reviewed well, but cannot be merged due to tests failing. and removed A0-pleasereview 🤓 Pull request needs code review. A8-looksgood 🦄 Pull request is reviewed well. labels Nov 22, 2016
@jacogr
Copy link
Contributor

jacogr commented Nov 22, 2016

CI tests still seems to have issues, will get back to this once recovered.

@ngotchac
Copy link
Contributor Author

Yep, no space left on device...

@jacogr jacogr added A8-looksgood 🦄 Pull request is reviewed well. and removed A7-looksgoodtestsfail 🤖 Pull request is reviewed well, but cannot be merged due to tests failing. labels Nov 22, 2016
@gavofyork gavofyork merged commit 490b39c into master Nov 22, 2016
@gavofyork gavofyork deleted the ng-fast-ui branch November 22, 2016 16:05
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants