Remove unneeded selectors from stats/utils #21641
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
It's only used in `PostTrends.shouldComponentUpdate` as an optimization attempt to reduce rerenders. The changes in #21507 achieve that goal better, though: by selector memoization and deep comparison of the `query` prop. The `shouldComponentUpdate` and the associated selector is no longer needed. This patch even solves a bug where the component wouldn't get rerendered when the `state.canScrollLeft` and `state.canScrollRight` flags changed, and the scrolling arrows on the "Posting Activity" chart wouldn't get the `is-active` CSS class.
jsnajdr
added
[Feature] Stats
samouri
reviewed
Jan 18, 2018
| startDate; | ||
| const { streakData } = this.props; | ||
| // Compute maximum per-day post count from the streakData. It's used to scale the chart. | ||
| const maxPosts = reduce( streakData, ( max, count ) => ( count > max ? count : max ), 0 ); |
Member
Author
There was a problem hiding this comment.
Good point, converted to max( values( streakData ) ), as streakData is an object with shape { 'date1': count1, 'date2': count2, ... }.
samouri
reviewed
Jan 18, 2018
| months = [], | ||
| startDate; | ||
| const { streakData } = this.props; | ||
| // Compute maximum per-day post count from the streakData. It's used to scale the chart. |
Contributor
samouri
approved these changes
Jan 18, 2018
Contributor
samouri
left a comment
samouri
left a comment
There was a problem hiding this comment.
I followed the testing instructions and everything worked as expected
timmyc
approved these changes
Jan 18, 2018
Contributor
timmyc
left a comment
timmyc
left a comment
There was a problem hiding this comment.
Testing out well for me, thanks for all the performance work you are doing in stats <3
Contributor
|
Unsetting & setting Needs Review label to force re-run of canaries with failing desktop canary now fixed. |
astralbodies
added
[Status] Needs Review
It computes the maximum per-day post count from `streakData`. This calculation is better done in the `render` method of `PostTrends`. There it's performed only when the component is rerendered, and not each time some irrelevant action is performed. Another little drive-by fix is using month index as a `key`. That's completely sufficient, no need to format date string.
It's not used anywhere.
jsnajdr
force-pushed
the
remove/stats-utils-unneeded-selectors
branch
from
d1b5703 to
74788df
Compare
sirreal
removed
the
[Status] Needs Review
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While working on #21507 and getting familiar with the Stats code, I realized that there are few selectors that can be removed. They are either not used, or can be replaced with something better:
Read the commit messages for details.
How to test:
Go to
/stats/insights/a8c.tvand play with the "Posting Activity" chart:When the width of the viewport is small, the chart should scroll and the scrolling arrows on the left and right site should switch between the "active" and "inactive" states. That verifies that the
PostTrendscomponent is rerendered when its state changes. That didn't work before, because itsshouldComponentUpdatewas broken and prevented the rerender.Also, verify that the "dots" in the chart use the full spectrum of colors. That's what the
getSiteStatsMaxPostsByDayselector used to help achieve and is now replaced by a computation directly in therendermethod.