-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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: send message - user navigates to Room tab when dragging to select e-mail from Chat tab #28240
Conversation
Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers! |
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.
Few comments other looks good.
@@ -129,6 +132,7 @@ const defaultProps = { | |||
icon: null, | |||
shouldUseDefaultValue: false, | |||
multiline: false, | |||
canInterceptSwipe: false, |
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.
Boolean props should be prefixed with should
or is
.
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.
Updated. Pushed a new commit.
}); | ||
|
||
export default responder; | ||
export default SwipeInterceptPanResponder; |
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.
The same things can be applied to MapView/responder/index.android.ts
.
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.
The responder in MapView/responder/index.android.ts
uses onStartShouldSetPanResponder
, not onMoveShouldSetPanResponder
, so that is different.
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.
Ohh I didn't see that 😅
Reviewer Checklist
Screenshots/VideosWeb28240.Web.mp4Mobile Web - Chrome28240.mWeb-Chrome.mp4Mobile Web - Safari28240.mWeb-Safari.mp4Desktop28240.Desktop.mp4iOS28240.iOS.mp4Android28240.Android.mp4 |
@akinwale There's an issue with Android. The text input on the chat tab won't be focused after swiping from the room tab. Screen_Recording_20230927_234832_New.Expensify.Dev.mp4 |
@mollfpr I am not able to reproduce this behaviour. Not sure what could be causing it as the responder shouldn't affect clicks or single taps, only movement gestures. Maybe the Android emulator was not responsive at that particular point in time? Could you try clicking elsewhere on the screen to see if the inputs register? Screencast.from.28-09-2023.05.02.49.webm |
@mollfpr bump. Any luck? |
@akinwale The issue I encountered doesn't reproduce on the emulator. The video I attached is using a Samsung physical device. Still no luck on the physical device 🥲 |
@mollfpr Not sure why Samsung devices are behaving differently here, but I've modified the code so that Pushed a new commit. |
Thanks @akinwale for the quick fix! I'll complete the test now! |
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.
LGTM and tests well 👍
}); | ||
|
||
export default responder; | ||
export default SwipeInterceptPanResponder; |
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.
What's the point of this file if all it does is export another file. Do we need it?
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.
My reasoning here was to try to maintain the responder
file structure since we have platform-specific implementations for web, iOS and Android. It's not "strictly" needed.
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.
@puneetlath Looks like I lied. The TypeScript Checks job fails if there is no index.ts
file present, so this is actually needed for now.
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.
Ok got it!
@puneetlath This did not get merged yet. Is that intentional? |
Whoops! Thanks for the bump. |
✋ 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/puneetlath in version: 1.3.78-0 🚀
|
🚀 Deployed to production by https://github.com/Beamanator in version: 1.3.78-4 🚀
|
🚀 Deployed to staging by https://github.com/puneetlath in version: 1.3.79-0 🚀
|
🚀 Deployed to production by https://github.com/jasperhuangg in version: 1.3.79-5 🚀
|
🚀 Deployed to production by https://github.com/jasperhuangg in version: 1.3.79-5 🚀
|
🚀 Deployed to production by https://github.com/jasperhuangg in version: 1.3.79-5 🚀
|
@@ -129,6 +132,7 @@ const defaultProps = { | |||
icon: null, | |||
shouldUseDefaultValue: false, | |||
multiline: false, | |||
shouldInterceptSwipe: false, |
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 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.
@Krishna2323 Just caught up on this. The default can be set to true
but you have to check to make sure that text selection still works as expected especially on Android and iOS native platforms.
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.
Sure, thanks a lot :)
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.
@Krishna2323 This is why it was set to false
in the first place: #28240 (comment)
You would need to test on a physical Samsung device.
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.
Then I guess its safe to fix only this instance. We are only using tab navigator in 2 places.
Details
Adds the
canInterceptSwipe
prop toBaseTextInput
which is used to stop a tab from swiping when interacting with a focused text input within the tab.Fixed Issues
$ #28047
PROPOSAL: #28047 (comment)
Tests
canUseAllBetas
to returntrue
.Offline tests
Same as tests.
QA Steps
Same as tests.
PR 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
28047-web.mp4
Mobile Web - Chrome
Mobile Web - Safari
28047-ios-safari.mp4
Desktop
28047-desktop.mp4
iOS
28047-ios-native.mp4
Android