-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
[HOLD for payment 2024-08-05] [$250] Bottom docked button views does not use correct SafeSpace in mobile #44056
Comments
Triggered auto assignment to @sakluger ( |
ProposalPlease re-state the problem that we are trying to solve in this issue.
What is the root cause of that problem?
What changes do you think we should make in order to solve the problem?
What alternative solutions did you explore? (Optional)
In there, |
@truph01 I think you have the right solution but I think we need to make sure we implement this on all pages that use the bottom-docked green button, not just the display name page. |
@shawnborton We can do it by remove |
ProposalPlease re-state the problem that we are trying to solve in this issue.Onboarding modal uses incorrect row hover style when hovering over the options What is the root cause of that problem?We disable safeAreaPadding for screens with forms as because it is already accounted for in Form Provider here This is also mentioned in docs here: App/contributingGuides/FORMS.md Lines 323 to 331 in 6dc6f81
We still seem to be missing the extra padding below the Form Submit button as we have not configured the styles correctly What changes do you think we should make in order to solve the problem?We will remove the bottom padding App/src/components/Form/FormWrapper.tsx Line 105 in 29e4c54
and apply it here: App/src/components/Form/FormWrapper.tsx Line 117 in 29e4c54
|
Cool, I agree that removing |
Job added to Upwork: https://www.upwork.com/jobs/~01cab23d89474f1097 |
Current assignee @allgandalf is eligible for the External assigner, not assigning anyone new. |
@allgandalf sure thing! |
Will review this today/tomorrow, reviewing an urgent deploy blocker today |
This comment was marked as outdated.
This comment was marked as outdated.
I feel both of the proposals don't solve the issue at root, I am keeping this one open for proposal updates until tomorrow @neonbhai @truph01 if you can review this bug again and think of a better solution, or will ask on slack for more help! You can go through my conversation with @shawnborton over the PR to understand more about the ideal solution |
@allgandalf Thanks for your feedback. Proposal updated |
I think both the proposals still don't solve the issue at root, according to the C+ doc:
🎀👀🎀 I would like to submit my proposal and get it reviewed by an internal engineer, if they agree with my proposal then i guess we need a new C+ and if they go with other proposals i can still review it. ProposalPlease re-state the problem that we are trying to solve in this issue.Form button is bottom docked, we need to have some padding added to it and not keep it close to the bottom. What is the root cause of that problem?This is a new requirement, we need to always have safe padding for forms. In our docs : App/contributingGuides/FORMS.md Lines 323 to 324 in 6dc6f81
We previously added this to prevent extra bottom padding for form buttons. What changes do you think we should make in order to solve the problem?The default value of App/src/components/ScreenWrapper.tsx Line 114 in 9d8e7fe
So it was only for So with the new requirement that we should always have bottom padding, we can remove the below condition: App/src/components/ScreenWrapper.tsx Lines 240 to 242 in 9d8e7fe
We will remove the if condition and always add padding. we should also now remove the usage of
What alternative solutions did you explore? (Optional)N/A |
Triggered auto assignment to @puneetlath, see https://stackoverflow.com/c/expensify/questions/7972 for more details. |
@allgandalf I think my solution and yours have the same implementation and output:
|
I felt that in your solution, you never mentioned that we should get ride of the condition all together because it is redundant and that is what i felt missing (i.e. issue is not solved at root), which is why i initially asked both of you to update your solutions. Also you mentioned to remove the use of Lets wait on the internal engineers comments now :) |
Proposal updated
|
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸 |
cool, thanks shawn, that’s what my proposal does 👍 |
|
Proposal updated
|
I updated comment |
Not overdue, Melvin, we're still discussing the proper solution. |
It sounds like we have pretty much a working solution and the rest can be worked out in the PR. I'll assign @truph01 and @allgandalf can work with him in the PR to make sure we're not introducing any regressions. Thanks y'all! |
📣 @allgandalf 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app! |
📣 @truph01 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app! Offer link |
@allgandalf PR #44761 is ready |
UpdateStill working through the PR, the updated changes are in a base component, so testing and working with the contributor to not cause any regression |
UpdatePR was deployed to staging, waiting to get it onto production |
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.13-4 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue: If no regressions arise, payment will be issued on 2024-08-05. 🎊 For reference, here are some details about the assignees on this issue:
|
BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:
|
All paid, thanks everyone. |
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: 1.4.86-0
Reproducible in staging?: y
Reproducible in production?: y
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: @shawnborton
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1718816465053499
Action Performed:
Expected Result:
There should be proper bottom SafeSpace below the button
Actual Result:
Bottom button is very close to the home bar
Workaround:
unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @Issue Owner
Current Issue Owner: @sakluger / @saklugerThe text was updated successfully, but these errors were encountered: