-
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: Pin & Delete request appear on Receipt image when quickly tap on receipt and 3-Dot menu #34720
Conversation
… receipt and 3-Dot menu
Reviewer Checklist
Screenshots/VideosAndroid: NativepindeleteAndroidNative.mp4Android: mWeb ChromepindeleteAndroidChrome.mp4iOS: NativepindeleteiOSNative.mp4iOS: mWeb SafaripindeleteiOSSafari.mp4MacOS: Chrome / SafaripindeleteChrome.mp4MacOS: DesktoppindeleteDesktop.mp4 |
@tienifr please attach videos of other platforms also. |
@c3024 done |
@@ -54,8 +54,8 @@ function setModalVisibility(isVisible: boolean) { | |||
* Allows other parts of app to know that an alert modal is about to open. | |||
* This will trigger as soon as a modal is opened but not yet visible while animation is running. | |||
*/ | |||
function willAlertModalBecomeVisible(isVisible: boolean) { | |||
Onyx.merge(ONYXKEYS.MODAL, {willAlertModalBecomeVisible: isVisible}); | |||
function willAlertModalBecomeVisible(isVisible: boolean, isPopover = 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.
Please add comment for why this param isPopover is required.
@@ -67,12 +76,14 @@ function ThreeDotsMenu({ | |||
shouldOverlay = false, | |||
shouldSetModalVisibility = true, | |||
disabled = false, | |||
modal = {}, | |||
}: ThreeDotsMenuProps) { | |||
const theme = useTheme(); | |||
const styles = useThemeStyles(); | |||
const [isPopupMenuVisible, setPopupMenuVisible] = useState(false); | |||
const buttonRef = useRef(null); | |||
const {translate} = useLocalize(); |
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.
NIT: In here
const isBehindModal = modal?.willAlertModalBecomeVisible && !modal?.isPopover && !shouldOverlay;
I think if modal?.willAlertModalBecomeVisible
is true
, the next ? after modal is not necessary.
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.
although we have default value {}
for modal but its type is modal: OnyxEntry, not modal?: Modal, and OnyxEntry can be null -> ? is necessary here
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.
I meant at line 86
const isBehindModal = modal?.willAlertModalBecomeVisible && !modal?.isPopover && !shouldOverlay;
it will reach to checking !modal?.isPopover
only when modal?.willAlertModalBecomeVisible
is true
which guarantees that modal
is not null
so in the modal?.isPopover
being evaluated only after modal?.willAlertModalBecomeVisible
is true
, modal check for null in modal?.isPopover
does not appear to be required.
It is a non issue and a very minor nitpick anyway 😁 . Need not be changed.
Regression here - the three dot menu is disappearing Screen.Recording.2024-01-25.at.10.30.47.PM.mov |
Please verify the three dot menus in all pages and all components where the field |
Please take extra care for these steps in the checklist
|
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.
Please fix the three dot menu regression attached in the video in the other comment.
It doesn't look like this is ready for me to review yet. Please ping me when it is! |
Sorry for missing checking three dot on native. I fixed Screen.Recording.2024-01-29.at.16.17.37.mov |
Testing well. However I am seeing a couple of warnings one on web and another on Android Native.
I don't think they are related to these changes. Nevertheless could you merge main so that we can just double check again? |
@c3024 After merge main. I don't face with these above warnings ^. I believe they're not related to this PR because we didn't change anythings about MenuItem or FlashList |
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
src/components/Popover/index.tsx
Outdated
@@ -69,6 +69,7 @@ function Popover(props: PopoverWithWindowDimensionsProps) { | |||
onLayout={onLayout} | |||
animationIn={animationIn} | |||
animationOut={animationOut} | |||
isPopover |
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 do we need this new prop? Isn't it redundant with the type
prop?
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.
Yah, I think we can remove it and use type. Thank you
@tienifr bump on my question. |
@puneetlath Thanks for your suggestion. We can lean on type instead of creating the new prop. I tested on all platforms and worked well. Can you help review again @c3024? |
Tested again Screenshots/VideosAndroid: NativepinDeleteAndroidNew.mp4Android: mWeb ChromepinDeleteAndroidChromeNew.mp4iOS: NativepinDeleteiOSNew.mp4iOS: mWeb SafaripinDeleteiOSSafariNew.mp4MacOS: Chrome / SafaripinDeleteChromeNew.mp4MacOS: DesktoppinDeleteDesktopNew.mp4 |
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.
Tests well.
✋ 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/puneetlath in version: 1.4.43-0 🚀
|
🚀 Deployed to production by https://github.com/puneetlath in version: 1.4.43-20 🚀
|
Details
Fixed Issues
$ #33541
PROPOSAL: #33541 (comment)
Tests
Offline tests
Same as above
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)/** comment above it */
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
Android: Native
Screen.Recording.2024-01-25.at.15.26.52.mov
Android: mWeb Chrome
Screen.Recording.2024-01-25.at.15.13.44.mov
iOS: Native
Screen.Recording.2024-01-25.at.15.26.00.mov
iOS: mWeb Safari
Screen.Recording.2024-01-18.at.17.45.00.mov
MacOS: Chrome / Safari
Screen.Recording.2024-01-18.at.17.42.58.mov
MacOS: Desktop
Screen.Recording.2024-01-18.at.18.27.42.mov