Skip to content
This repository has been archived by the owner on Mar 13, 2024. It is now read-only.

PLT-8208 Use ref callback to set focus on channel switcher #342

Merged
merged 2 commits into from
Nov 21, 2017

Conversation

jwilander
Copy link
Member

Summary

It looks like this was caused by the upgrade to React 16. For some reason the component refs isn't getting set in time for componentDidUpdate(), when in the past it would.

There are some mentions in the changelog about changes to lifecycle and refs https://github.com/facebook/react/releases/tag/v16.0.0 but nothing that I can see that should directly affect this behavior and all the docs I can find seem to state that refs should be available in time for componentDidUpate().

Fixed by making use of the ref callback.

Ticket Link

https://mattermost.atlassian.net/browse/PLT-8208

@jwilander jwilander added the 2: Dev Review Requires review by a core commiter label Nov 21, 2017
@@ -311,7 +309,10 @@ export default class QuickSwitchModal extends React.PureComponent {
{help}
</div>
<SuggestionBox
ref='switchbox'
ref={(input) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine with this but perhaps you want to create a function like

suggestionBoxRef = (ref) => {
    this.switchBox = ref;
    this.focusTextbox();
}

and then here just do ref={this.suggestionBoxRef} just to avoid creating a new function each time this component renders

Copy link
Contributor

Choose a reason for hiding this comment

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

a nice article for why we should avoid arrow functions and binds like this one https://medium.freecodecamp.org/why-arrow-functions-and-bind-in-reacts-render-are-problematic-f1c08b060e36

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, but all the React docs show creating a new function :P

@jwilander
Copy link
Member Author

@enahum change made

@enahum enahum added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core commiter labels Nov 21, 2017
@jwilander jwilander merged commit 58cdc92 into master Nov 21, 2017
@jwilander jwilander deleted the plt-8208 branch November 21, 2017 17:24
@lindalumitchell lindalumitchell added the Tests/Done Release tests have been written label Nov 27, 2017
@esethna esethna added Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation labels Dec 4, 2017
hmhealey pushed a commit that referenced this pull request Aug 28, 2020
* Add timeout flags for mobile

* Wait 10 seconds before failing
hmhealey pushed a commit that referenced this pull request Mar 17, 2021
* Add timeout flags for mobile

* Wait 10 seconds before failing
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
4: Reviews Complete All reviewers have approved the pull request Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation Tests/Done Release tests have been written
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants