Focus emoji picker search input #2429
Conversation
# Conflicts: # src/pages/home/report/ReportActionCompose.js
roryabraham
changed the title
roryabraham
changed the title
| forwardedRef: () => {}, | ||
| }; | ||
|
|
||
| class WithWindowDimensions extends Component { |
There was a problem hiding this comment.
Should be camelCase I think
There was a problem hiding this comment.
I actually had to change this to PascalCase so that it can be rendered like <WithWindowDimensions />. I guess if you try and do <withWindowDimensions />, react will think you're trying to render regular html like <input> or <table> or whatever and complain that it doesn't support the <withWindowDimensions /> tag. Maybe I was just holding something wrong, but changing it to PascalCase fixed the error.
Anyways, the good news is that it uses a default export so it can still be imported and used in camelCase.
There was a problem hiding this comment.
Huh, how strange.
Maybe we should change our code style to capitalize components. All of the ones in the react native docs are PascalCase...
There was a problem hiding this comment.
I wonder why it was working before but not now. Can you account for why that discrepancy is occurring?
There was a problem hiding this comment.
Short answer:
Before, we weren't ever explicitly rendering it like <withWindowDimensions />, but now we are explicitly rendering it on line 86.
Long Answer:
withWindowDimensions as you know is a HOC, which means that it is a function that takes a component as input and returns a component. You'll notice that the WrappedComponent is using PascalCase, and is rendered starting on line 69.
Before, we weren't doing any explicit rendering of the withWindowDimensions component itself, but rather calling it as a function, getting a component back, and rendering that.
We are able to pass props through withWindowDimensions to the WrappedComponent by simply drilling them down (as done on line 62 before or line 71 now). But refs work differently. In order to pass a ref through to a wrapped component, we need to use React.forwardRef, so what we're doing now is something like this:
React.forwardRef(withWindowDimesions(WrappedComponent))
But in order to forward the ref through withWindowDimensions, withWindowDimensions needs to accept the ref as a prop and then pass it as a ref to the WrappedComponent. So that's why we're explicitly rendering <WithWindowDimensions /> on line 86 - so that we can pass the forwarded ref as a prop.
Caveat: I admit this feels a bit weird but I can't think of any other way to pass a ref through a HOC to a wrapped component. Open to any ideas if there's an easier way to do this.
Reference for where I found this solution: https://dev.to/justincy/using-react-forwardref-and-an-hoc-on-the-same-component-455m
There was a problem hiding this comment.
Maybe we should change our code style to capitalize components
Well, that is normally how we do this, except all our HOCs use camelCase by convention. This still uses camelCase everywhere else in the code and works just the same, except it can now accept a ref and pass it through.
There was a problem hiding this comment.
Ah ok, thanks for this explanation! Very nice ❤️
There was a problem hiding this comment.
Just passing through, but I agree with Luke that this should follow the camel case convention like other HOC.
My 2 cents, it would have been simpler to just drill the withWindowDimensions props down through EmojiPickerMenu. Instead we increased the complexity of a very general thing to cover a use case that we will very rarely run into.
| WithWindowDimensions.propTypes = propTypes; | ||
| WithWindowDimensions.defaultProps = defaultProps; | ||
| WithWindowDimensions.displayName = `withWindowDimensions(${getComponentDisplayName(WrappedComponent)})`; |
There was a problem hiding this comment.
Same here on the camelCase comment
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to production in version: 1.0.39-5🚀
|
Details
Following-up on #1991 (comment). During testing, we found a minor issue that when the
EmojiPickerMenuis first opened, neither theReportActionComposenor the emoji search input are focused. We decided that because that was already a big PR it would be easier to handle the focus issue in a follow-up PR.Note: I also found an issue with the existing implementation where the search input was displayed on mobile web, which we don't want for the same reasons we don't want it to display on mobile (i.e: the native keyboard competes with the emoji picker.
Fixed Issues
n/a.
Tests / QA Steps (Web/Desktop)
ReportActionComposeto open the emoji picker.ReportActionCompose(make sure it doesn't take two clicks)ReportActionComposehas focus again.Tests / QA Steps (iOS/Android/mWeb)
Just make sure that the emoji picker still works like normal. This PR should have no other effect.
Tested On
Screenshots
Web
Mobile Web
Desktop
iOS
Android