-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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 can't save workspace name using ENTER key after creating a new workspace from workspace switcher page #37069
Fix can't save workspace name using ENTER key after creating a new workspace from workspace switcher page #37069
Conversation
@@ -125,8 +125,11 @@ class BaseOptionsSelector extends Component { | |||
// Unregister the shortcut before registering a new one to avoid lingering shortcut listener | |||
this.unSubscribeFromKeyboardShortcut(); | |||
if (this.props.isFocused) { | |||
this.subscribeActiveElement(); |
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.
why is this 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.
To resubscribe when the screen is refocused, otherwise, active element listener won't work anymore.
Reviewer Checklist
Screenshots/VideosAndroid: NativeScreen.Recording.2024-03-03.at.11.10.50.PM.movAndroid: mWeb ChromeScreen.Recording.2024-03-03.at.8.22.19.in.the.evening.moviOS: NativeScreen.Recording.2024-03-03.at.10.40.30.PM.moviOS: mWeb SafariScreen.Recording.2024-02-23.at.8.56.25.AM.movMacOS: Chrome / SafariScreen.Recording.2024-02-23.at.8.48.32.AM.movMacOS: DesktopScreen.Recording.2024-03-03.at.8.05.52.in.the.evening.mov |
@bernhardoj pressing enter reloads the app on android mWeb. Screen.Recording.2024-02-23.at.9.01.38.AM.mov |
Oh yeah, I can reproduce it when using the soft keyboard button. Most likely related to #36401, but after applying the solution there, I can't use ENTER anymore on mWeb (maybe it's expected based on this comment). cc: @shubham1206agra |
@bernhardoj Can you help me give context related to this component you are trying to fix, and I will tell you the solution to this? |
@shubham1206agra The issue we have here is that the ENTER key doesn't work after creating a new workspace from the workspace switcher page because the ENTER event is still captured in BaseOptionsSelector. @getusha then found that pressing ENTER on Android mWeb reloads the web. But since this happens on main too, I think we can still proceed with the PR. |
Nope, this is not ideal @bernhardoj. You may need to fix this if this PR changes the previous behaviour. The ideal behaviour was for that component only, not in general. |
I think there is a bit of misunderstanding here, i think @bernhardoj meant to say after applying the solution from your PR. |
@getusha +1, also it's not limited to this page, but to every page that is wrapped with a form |
@bernhardoj turns out only applying this change will fix the refresh issue, could you check if we can apply it? index dc2f8ebb45..f6beb569a9 100644
--- a/src/pages/workspace/WorkspaceNamePage.tsx
+++ b/src/pages/workspace/WorkspaceNamePage.tsx
@@ -71,6 +71,7 @@ function WorkspaceNamePage({policy}: Props) {
validate={validate}
onSubmit={submit}
enabledWhenOffline
+ disablePressOnEnter={false}
>
<View style={styles.mb4}>
<InputWrapper |
Please don't apply this workaround. There seems to be another issue since |
@shubham1206agra could you explain a bit more, on why? i also see it being used in your PR https://github.com/Expensify/App/pull/36401/files#diff-5e02d6b48ee46a9077d2d4f5db2f7253199d570550fb4d81a906ff2b1bcd9b0eR95 |
That is a special case, as it uses BNP on mobile devices. So there's no Enter key. And one more thing, it was not used for handling refresh. It was to enable enter submission for web devices. In your case, please try using the same in Native devices. It might not work as expected. |
@bernhardoj Looks like @shubham1206agra's PR is merged, can you pull main and retest please? |
Merged with main. As mentioned before, can't use Enter from soft keyboard Screen.Recording.2024-03-01.at.11.03.19.movHappens on other forms Screen.Recording.2024-03-01.at.11.03.47.mov |
✋ 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 https://github.com/francoisl in version: 1.4.47-0 🚀
|
🚀 Deployed to production by https://github.com/roryabraham in version: 1.4.47-10 🚀
|
Details
The ENTER key is captured by the workspace switcher page ENTER key shortcut. The shortcut shouldn't be active anymore and this PR fix it.
Fixed Issues
$ #36923
PROPOSAL: #36923 (comment)
Tests
Same as QA Steps
Offline tests
Same as QA Steps
QA Steps
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)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label so the design team can review the changes.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
Android: Native
Screen.Recording.2024-02-22.at.13.34.06.mov
Android: mWeb Chrome
Screen.Recording.2024-02-22.at.13.34.52.mov
iOS: Native
Screen.Recording.2024-02-22.at.13.36.17.mov
iOS: mWeb Safari
Screen.Recording.2024-02-22.at.13.35.36.mov
MacOS: Chrome / Safari
Screen.Recording.2024-02-22.at.13.31.37.mov
MacOS: Desktop
Screen.Recording.2024-02-22.at.13.32.47.mov