-
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
[$250] [Dev][StrictMode] Popover menu has no background smallscreen #45385
Comments
Triggered auto assignment to @lschurr ( |
This has been labelled "Needs Reproduction". Follow the steps here: https://stackoverflowteams.com/c/expensify/questions/16989 |
Job added to Upwork: https://www.upwork.com/jobs/~01ff564d543a9355a6 |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @allroundexperts ( |
I'm not able to test since I don't work in a DEV environment. @allroundexperts could you try reproducing this? |
It's easily reproducible @lschurr. Screen.Recording.2024-07-16.at.12.05.09.AM.mov |
ProposalPlease re-state the problem that we are trying to solve in this issue.Popover menu has no background in small screens in the development environment when using the strict mode. What is the root cause of that problem?
What changes do you think we should make in order to solve the problem?Using the
in line 209 at: App/src/components/Modal/BaseModal.tsx Lines 205 to 218 in 105dce5
then bind the ref to the ReactNativeModal at line 224 in the following code snippet:App/src/components/Modal/BaseModal.tsx Lines 220 to 230 in 105dce5
Below is a screenshot after applying the change: What alternative solutions did you explore? (Optional)The last commit to the React Native Modal library used in the Popover menu is 2 years old and is looking for maintainers, which is mentioned in https://github.com/react-native-modal/react-native-modal?tab=readme-ov-file#announcements, so we can use the React Native built-in |
ProposalPlease re-state the problem that we are trying to solve in this issue.Popover menu don't have background when strict mode is enabled in dev env What is the root cause of that problem?We have 2 problems here in dev strict mode, and I'll try to fix both:
For first problem, the issue is because in strict mode, React will try to simulate mount -> unmount -> remount the component, and during that simulation, the The second problem has the same root cause, the difference is that the final computed style is correct, but this animation trigger doesn't run on the remount simulation, the context menu seems to pop-up right away instead of a smooth transition What changes do you think we should make in order to solve the problem?We either can patch or contribute to the original package, but the idea is that:
What alternative solutions did you explore? (Optional)We can consider using a custom backdrop to manage the opacity transition ourselves, which allow us to provide a robust implementation that satisifies the strict mode. However, the second problem remains (in StrictMode dev env only), but if we consider it non-important, we can go with this approach. |
Thanks for the proposal @dominictb. Your RCA seems to be accurate, but I'm not sure if patching / editing the package is the correct way to proceed here. Software mansion maintains the react-native-reanimated library, and I think it would be great if we can consult about this with them in the open source channel. Can you spin up a message and tag me? Thanks! |
@isogit123 Your RCA is incomplete. Please make sure to include proper root cause in order for the proposal to be accepted. |
Open a discussion in Slack https://expensify.slack.com/archives/C01GTK53T8Q/p1721299326263619 |
@allroundexperts sorry but I think we are referring to 2 different libraries here. The package mentioned in my proposal is |
Any update on this one @allroundexperts? |
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸 |
Yes, my bad. Thinking about it now, can we just use react-native-reanimated only here? |
@allroundexperts
|
@allroundexperts, @lschurr Uh oh! This issue is overdue by 2 days. Don't forget to update your issues! |
Bump on this @allroundexperts |
@allroundexperts we got 1 patch for We are using |
Okay. Can we somehow not use |
We still need to patch |
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸 |
@allroundexperts @lschurr this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks! |
Ah, I see. I guess that's the best option then. |
@dominictb's proposal looks good to me. Let's create a patch for the package. 🎀 👀 🎀 C+ reviewed |
Triggered auto assignment to @flodnv, see https://stackoverflow.com/c/expensify/questions/7972 for more details. |
📣 @dominictb 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app! Offer link |
On it today. |
PR should be up by today. |
Any update @dominictb? |
@flodnv, @allroundexperts, @lschurr, @dominictb Huh... This is 4 days overdue. Who can take care of this? |
Could you please review the PR @allroundexperts ? |
Reviewed! |
If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results. If a regression has occurred and you are the assigned CM follow the instructions here. If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future. |
This issue has not been updated in over 15 days. @flodnv, @allroundexperts, @lschurr, @dominictb eroding to Monthly issue. P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do! |
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Version Number:
Reproducible in staging?: DEV only
Reproducible in production?: DEV only
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: @ishpaul777
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1720819445275459
Action Performed:
Long press on any report in LHN
Expected Result:
Popover menus haves translucent background
Actual Result:
Popover menu dont have background
Workaround:
unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
Screen.Recording.2024-07-13.at.2.49.31.AM.mov
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @allroundexpertsThe text was updated successfully, but these errors were encountered: