-
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: Web - Split - Enter and CMD+Enter open user profile instead of splitting expense. #43794
Conversation
…plitting expense. Signed-off-by: Krishna Gupta <belivethatkg@gmail.com>
@mananjadhav Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
Signed-off-by: Krishna Gupta <belivethatkg@gmail.com>
@Krishna2323 Can you confirm if this is resolved and it's ready for review? |
@mananjadhav, no that's not resolved, still looking for a solution, I will explain the problem if I couldn't resolve it today. |
When the dropdown button has multiple option, we are not setting the App/src/components/ButtonWithDropdownMenu/index.tsx Lines 75 to 77 in 633e708
But when we have simple button the
Why do we need the App/src/components/KYCWall/BaseKYCWall.tsx Lines 164 to 165 in 633e708
|
@mananjadhav, Directly setting the ref like App/src/components/ButtonWithDropdownMenu/index.tsx Lines 75 to 77 in 633e708
Setting the ref={(ref) => {
caretButton.current = ref;
buttonRef.current = ref;
}} |
Signed-off-by: Krishna Gupta <belivethatkg@gmail.com>
Pls checkout 7c4f38e |
@Krishna2323 Using |
@Krishna2323 Is this ready for review? Can you please sync the latest main here? |
@mananjadhav, this is ready, I will check the eslint rule in few moments. |
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
@mananjadhav, |
The code change looks fine to me. Will get this tested today. |
Reviewer Checklist
Screenshots/VideosAndroid: Nativeandroid-split-on-enter.movAndroid: mWeb Chromemweb-chrome-split-on-enter.moviOS: Nativeios-split-on-enter.moviOS: mWeb Safarimweb-safari-split-on-enter.movMacOS: Chrome / Safariweb-split-on-enter-1.movweb-split-on-enter-2.movMacOS: Desktopdesktop-split-on-enter-1.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.
@Krishna2323 thanks, can you please add that one missing doc?
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
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! @mananjadhav @Krishna2323 lets be on a look out for regressions, these PRs are often tricky
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
Hey, this is preventing drop down buttons to work entirely Screen.Recording.2024-07-22.at.17.31.32.movReverting it here: #45964 |
🚀 Deployed to staging by https://github.com/mountiny in version: 9.0.11-0 🚀
|
🚀 Cherry-picked to staging by https://github.com/francoisl in version: 9.0.11-2 🚀
@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes. |
} | ||
caretButton.current = buttonRef.current; | ||
}, [buttonRef, options.length, shouldAlwaysShowDropdownMenu]); | ||
|
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.
@mananjadhav @mountiny, this code caused the regression, I have found the solution and will raise PR today. We need to use mergeRefs instead of this.
🚀 Deployed to production by https://github.com/francoisl in version: 9.0.11-5 🚀
|
🚀 Deployed to production by https://github.com/francoisl in version: 9.0.12-0 🚀
|
Details
Fixed Issues
$ #41848
PROPOSAL: #41848 (comment)
Tests
Offline tests
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 methodSTYLE.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 and/or tagged@Expensify/design
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
android_native.mp4
Android: mWeb Chrome
android_chrome.mp4
iOS: Native
ios_native.mp4
iOS: mWeb Safari
ios_safari.mp4
MacOS: Chrome / Safari
web_chrome.mp4
MacOS: Desktop
desktop_app.mp4