[webapp-perf] Change anonymous inline function into a defined arrow function #484
Conversation
| @@ -238,8 +242,8 @@ export default class MultiSelect extends React.Component { | |||
| onChange={this.onChange} | |||
| value={this.props.values} | |||
| valueRenderer={this.props.valueRenderer} | |||
| menuRenderer={() => null} | |||
| arrowRenderer={() => null} | |||
| menuRenderer={this.handleRender} | |||
There was a problem hiding this comment.
No need for a function here, actually you could even remove the props
There was a problem hiding this comment.
I've tried to remove those but
- when
menuRendereris removed, the menu opened up as soon as the user access a component with MultiSelect (see screenshot)
- when
arrowRenderer, an extra caret show up in the text box
So I'll keep it that way.
components/navbar/navbar.jsx
Outdated
| @@ -354,6 +354,10 @@ export default class Navbar extends React.Component { | |||
| ); | |||
| } | |||
|
|
|||
| hideHeaderOverlay = () => { | |||
| this.refs.headerOverlay.hide(); | |||
There was a problem hiding this comment.
what I mean here is that you should wrap this in an if statement like
hideHeaderOverlay = () => {
if (this.refs.headerOverlay) {
this.refs.headerOverlay.hide();
}
};There was a problem hiding this comment.
I totally missed this one. Should be good now.
components/navbar/navbar.jsx
Outdated
| } | ||
|
|
||
| showChannelInviteModalButton = () => { | ||
| this.refs.channelInviteModalButton.show(); |
| @@ -144,6 +144,11 @@ export default class NewChannelModal extends React.PureComponent { | |||
| this.props.onDataChanged(newData); | |||
| } | |||
|
|
|||
| handleOnURLChange = (e) => { | |||
| e.preventDefault(); | |||
| this.props.onChangeURLPressed(); | |||
There was a problem hiding this comment.
Is the prop required? If not check that is defined
| @@ -211,6 +211,10 @@ export default class RhsRootPost extends React.Component { | |||
| }); | |||
| } | |||
|
|
|||
| getDotMenuRef = () => { | |||
| return this.refs.dotMenu; | |||
There was a problem hiding this comment.
Just add check at consuming component - EmojiPickerOverlay
There was a problem hiding this comment.
@saturninoabril Have you ensured that every component you're changing the prop for fits one of the following:
a) Is a PureComponent, or
b) Is otherwise leveraging the prop with the non-inline function in its shouldComponentUpdate method?
The reason I ask is that if neither of those two cases are met then this performance optimization won't be realizing its full potential.
|
@mkraft This PR is mainly an outcome of what was discussed (and I supposed was agreed upon) last performance meeting about inline function. A similar follow up discussion here - https://pre-release.mattermost.com/core/pl/s4yfjinzhby38887au35yn8yzo |
|
@saturninoabril I agree with what you're saying and maybe I should address the |
|
Rebase to latest master and fix merge conflicts |
|
@saturninoabril the rest of the code looks good, but I don't feel very comfortable to merge it for 4.6 as is very late in the game and that PR looks sensible and it does not give an important performance boost, what do you think? |
Summary
data-attrin storing information which serves as parameter of underlying function.Note: This is second installment PR. Other PR submitted:
Ticket Link
[Please link the GitHub issue or Jira ticket this PR addresses.]
Checklist
make check-styleto check for style errors (required for all pull requests)make testto ensure unit and component tests passed