PR 2: Made EmojiPicker a Singleton Component #7578
Conversation
MelvinBot
requested review from
parasharrajat and
puneetlath
and removed request for
a team
mananjadhav
changed the title
|
Waiting for First PR to merge.... |
|
@parasharrajat I'll pull the main branch and update this PR for you to review. |
…singleton # Conflicts: # src/pages/home/report/ReportActionCompose.js
|
@parasharrajat @puneetlath PR ready for your review. |
…moji-picker-singleton
…moji-picker-singleton
src/components/EmojiPicker/index.js
Outdated
| width: CONST.EMOJI_PICKER_SIZE, | ||
| height: CONST.EMOJI_PICKER_SIZE, |
There was a problem hiding this comment.
EMOJI_PICKER_SIZE indicates the width of it which can't be used for height. we have to give a static to the picker and create another variable for the height. Could you please ask on the slack what should be the height of the picker? I think 380 - 400 px looks good to me.
we can define Const as:
EMOJI_PICKER: {
WIDTH: 320,
HEIGHT: 380
}
then apply the height to Picker and use to pass to the dimensions.There was a problem hiding this comment.
Okay I can confirm, but when I saw the height in my browser, it was 322, hence I used the same var.
There was a problem hiding this comment.
It seems the height was 405 for the Popover. I've used 400 and it looks fine. Do we still need to check on Slack?
There was a problem hiding this comment.
Yes, please. I am not a design expert. @shawnborton Could you please help us determine the correct height for EmojiPicker?
There was a problem hiding this comment.
Hmm I'm honestly not too sure, but 400 seems fine to me?
There was a problem hiding this comment.
It is very important that you @mananjadhav take care of the case where screen height is less than 400 then it should just take screen height as before and respect all the margins that are applied
There was a problem hiding this comment.
take care of the case where screen height is less than 400 then it should just take screen height as before and respect all the margins that are applied
I am just testing everything right now.
There was a problem hiding this comment.
then it should just take screen height as before
Was it working before? Afai can tell, the current picker still gets cut when the screen height is less than 400 on the web. For mobile devices, anyway, we have a bottom docked modal.
Except for the height issue on the web, it is working fine on all the platforms.
Co-authored-by: Rajat Parashar <[email protected]>
…njadhav/App into feat/emoji-picker-singleton
|
PR updated. @parasharrajat @puneetlath |
|
✋ 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 staging by @puneetlath in version: 1.1.40-0 🚀
|
|
🚀 Deployed to production by @yuwenmemon in version: 1.1.40-2 🚀
|
|
Issue 1 - Title- [High]: Chrome + Jaws : Keyboard : 'Change skin tone' control is not accessible using keyboard. 7578_Chnage.skintone.is.not.accessibl.using.keyoard.mp4Issue 2 - Title- [Medium]: Chrome + Jaws : Screen Reader: Name & Role is not defined for the 'Emoji' control. 7578_Name.role.is.not.defined.for.Emoji.control.mp4Issue 3 - Title- [Medium]: Chrome + Jaws : Keyboard: 'Emoji' control is not accessible using screen reader. 7598_Emoji.control.is.not.accessible.using.screen.reader.mp4 |
Details
This is the second PR for the same tagged issue, which makes the EmojiPicker a singleton component by keeping a common ref. Refer the comment [HOLD for payment 2022-04-12] [$2000] Edit Comment - There's no emoji picker when editing a comment #3245 (comment)
One change is that earlier we used to set props like
updatePreferredSkinTone, etc to the Popover child component. Because we're setting reference for thePopover, I've had to move these props toEmojiPickerMenuseparately toindex.jsandindex.native.jsAdditionally, changed the
PopoverwithPopoverWithMeasuredContentforEmojiPickerFixed Issues
$ #3245
Tests
QA Steps
Tested On
Screenshots
Web
Mobile Web
Desktop
iOS
Android