-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: prevent refocusing in options-selector and selection-list #29084
Conversation
src/components/OptionRow.js
Outdated
@@ -70,6 +70,9 @@ const propTypes = { | |||
/** Whether to remove the lateral padding and align the content with the margins */ | |||
shouldDisableRowInnerPadding: PropTypes.bool, | |||
|
|||
/** Whether to take focus when selecting */ | |||
shouldTakeFocus: PropTypes.bool, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make the name consistent? So use shouldFocusOnSelectRow
and reverse conditions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes we can, but it looks weird considering the name shouldFocusOnSelectRow
as OptionRow
's props
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When we should refocus on selecting row for options-selector, the option row should not take focus. I thought this makes sense.
Do you still want to rename the props?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, tricky naming. shoudTakeFocus
is fine but no one knows what this does when read this code without any context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe shouldPreventDefaultFocusOnClick
though it's long name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldPreventDefaultFocusOnSelect
is comparatively near to what we do in the code. So I would vote for it first
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we still want to have the same props name, I suggest shouldPreventDefaultFocusOnSelectRow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldPreventDefaultFocusOnSelectRow
sounds good to me.
I think also shouldPreventDefaultFocusOnSelectItem
will work.
But yes, using same prop through out the tree will be better IMO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay. Let me rename the props for BaseOptionSelector, BaseOptionsList, BaseSelectionList, BaseListItem, and OptionRow
cc @situchan
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay. Let me rename the props for BaseOptionSelector, BaseOptionsList, BaseSelectionList, BaseListItem, and OptionRow
cc @situchan
👍
@s-alves10 let's also fix invite page Screen.Recording.2023-10-10.at.12.01.08.AM.mov |
Workspace invite page was updated. Videos were updated as well. Please take a look |
@s-alves10 please correct QA Steps so that QA team doesn't get confused
|
I'm sorry I forgot to update that. Just updated |
Reviewer Checklist
Screenshots/VideosWebweb.movMobile Web - Chromemchrome.movMobile Web - Safarimsafari.movDesktopdesktop.moviOSios.movAndroidandroid1.movandroid2.mov |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MonilBhavsar all yours
Will you take a look? |
Yes, reviewing now. |
@@ -231,6 +232,7 @@ function WorkspaceInvitePage(props) { | |||
onConfirm={inviteUser} | |||
showScrollIndicator | |||
showLoadingPlaceholder={!didScreenTransitionEnd || !OptionsListUtils.isPersonalDetailsReady(props.personalDetails)} | |||
shouldFocusOnSelectRow={!Browser.isMobile()} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are two props - shouldFocusOnSelectRow
and shouldPreventDefaultFocusOnSelect
. Can we make it one to make it easy to understand?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MonilBhavsar Can you please check this discussion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Commented there
Props name updated. Please check |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
Performance Comparison Report 📊Significant Changes To Duration
Show details
Meaningless Changes To DurationShow entries
Show details
|
@Expensify/mobile-deployers 📣 Please look into this performance regression as it's a deploy blocker. |
🚀 Deployed to staging by https://github.com/MonilBhavsar in version: 1.3.81-0 🚀
|
🚀 Deployed to staging by https://github.com/MonilBhavsar in version: 1.3.83-0 🚀
|
Details
Fixed Issues
$ #22812
PROPOSAL: #22812 (comment)
Tests
Send message
Add to group
buttonRequest money
Next
Split
buttonOffline tests
Send message
Add to group
buttonRequest money
Next
Split
buttonQA Steps
Note: All the tests should be done on ios/android native, web, and desktop platforms. In mobile web platforms, search input gets blurred on selection by intention
Send message
Add to group
buttonRequest money
Next
Split
buttonPR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Web
22812_mac_chrome.mp4
Mobile Web - Chrome
22812_android_chrome.mp4
Mobile Web - Safari
22812_ios_safari.mp4
Desktop
22812_mac_desktop.mp4
iOS
22812_ios_native.mp4
Android
22812_android_native.mp4