-
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 Composer context menu not opening #55647
base: main
Are you sure you want to change the base?
Conversation
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.
contextMenuHidden={getPlatform() === CONST.PLATFORM.IOS ? contextMenuHidden : false}
@QichenZhu I thought the only changes we need are on src/components/Composer/implementation/index.native.tsx
, but I see other commits that I think are not related to the issue. For example the changes to use destructed variable instead of the dot notation.
Also, what issue you facing to update the storybook and calendar picker unit test?
@mollfpr I understand this PR is bigger than expected and difficult to review, but these changes are required by the process.
|
Hey! Looks like most of the changes were needed in order to have the lint pass. @QichenZhu Regarding the Safari issue, I really appreciate the proactivity in tackling this, but I feel like we should at least update the original proposal to add the changes so it is documented. Besides that, please avoid doing any refactoring that are not necessary (either by lint or changes affecting unit tests), I'm not sure how this PR would impact the CalendarPicker tests, if they were not impacted by these changes, perhaps we should check if they are Ok in main and merge main before proceeding. |
@MarioExpensify Thanks for the advice! I’ve updated my proposal. Regarding CalendarPicker tests, they previously blocked this PR as well as other PRs, so I made the fix and shared it on Slack. Then the original author of the tests picked the fix into main. |
@QichenZhu Nice, so do you mind reverting the calendar test changes and merging main to your branch? Thank you. |
Done. |
Explanation of Change
Fixed Issues
$ #50682
PROPOSAL: #50682 (comment)
Tests
Precondition: Copy some text to the clipboard.
Offline tests
Precondition: Copy some text to the clipboard.
QA Steps
Precondition: Copy some text to the clipboard.
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectionTwo issues have already been reported:
Mac/Safari - [AU] - "Fetch API cannot load" and "interactive-widget" console errors when reloading #54639
[Console Errors] Console Error Cleanup part trois... or tre... or.... #55045
toggleReport
and notonIconClick
)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.I didn't add unit tests because the original issue needs testing on Android and iOS Safari, not the testing framework environment.
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-web.mp4
iOS: Native
ios-native.mp4
iOS: mWeb Safari
ios-web.mp4
MacOS: Chrome / Safari
mac-web.mov
MacOS: Desktop
mac-desktop.mov