-
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
[$250 - WAITING FOR PAYMENT] IOS - Submit expense - Next button not fully visible after returning online #41810
Comments
Triggered auto assignment to @sonialiap ( |
@sonialiap FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors |
We think that this bug might be related to #wave-collect - Release 1 |
ProposalPlease re-state the problem that we are trying to solve in this issue.Next button not fully visible after returning online What is the root cause of that problem?
What changes do you think we should make in order to solve the problem?We will pass |
Job added to Upwork: https://www.upwork.com/jobs/~01f1a3e2d4eb573fb6 |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @eVoloshchak ( |
ProposalPlease re-state the problem that we are trying to solve in this issue.Native small screens - Submit expense - Next button not fully visible after returning online What is the root cause of that problem?Currently when we are using "small screens" (or native screens) the MapView try to use the space available to render itself, this space is calculated by the ScreenWrapper component based in the initialHeight (in this case) and the children components. The issue is for native screens when we render the map the first time (when online) we take the all available space, but then when we go offline we show/render an offline indicator, this makes the available height reduced (so we see the map pending indicator a bit short than the map), then when we go online the map try to recover its original height (but now we have less height space due the offline indicators) and move down the next button. What changes do you think we should make in order to solve the problem?We should set the height of each container with percents to fix this issue in IOURequestStepDistance and also in MoneyRequestAmountForm, to keep next button aligned. The changes should be in
And in
Also in App/src/pages/iou/MoneyRequestAmountForm.tsx Lines 302 to 306 in 66b4072
And finally in App/src/pages/iou/MoneyRequestAmountForm.tsx Line 314 in 66b4072
This way we going to force to react native to recalculate the height of the component even when offline indicator are not displayed (when they doesn't exists in the rendered components). Tests:Before changes: Map.view.overlap.next.button.-.test.before.changes.mp4After changes next.button.moved.test.mp4 |
@eVoloshchak, @sonialiap Huh... This is 4 days overdue. Who can take care of this? |
@eVoloshchak what do you think of the above proposal? |
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸 |
@eVoloshchak, @sonialiap 6 days overdue. This is scarier than being forced to listen to Vogon poetry! |
@neonbhai, I've applied your proposal to @samilabud, thank you for the thorough explanation!
Can we make the map view fill all the available space? |
Good point, I will try to do a formula to calculate the height based to the current calculation that we do in ScreenWrapper, this way I think this going to persist good in the future. |
At the end the only way I found was using flexbox, I have updated my proposal, please check it out 🙏🏼: |
@eVoloshchak, @sonialiap Whoops! This issue is 2 days overdue. Let's get this updated quick! |
@jscoder2151, I've applied your solution, but it seems there is an issue with the Next button placement |
@samilabud, same issue with your updated proposal, I do think this might be due to recent changes to Forms component on main |
@eVoloshchak @sonialiap 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! |
Please let me know if we should roll back the upgrade and use |
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. |
@Julesssss thoughts on this having a regression? |
Hey @sonialiap, I don't think we need a test for this specific case |
@Julesssss sorry, I was unclear. This issue is linked to a deploy blocker so I was looking to clarify if this issue had a regression |
@sonialiap I don't think so. So @QichenZhu's proposal was correct, but due to other bugs in the upstream library (and a combination of the large RN version bump) we had to revert the change. I think the next step should be to decide on one of the following options: @QichenZhu does the first option sound good to you? If so we can double the bounty due to the time/effort spent on this issue. |
@Julesssss Yes, I'm happy to go with the first option. Thank you very much! |
Thanks, Jules :) |
#48169 already bumped the library version, so no PR is needed here. |
@Julesssss Qichen mentioned that the library was bumped. What should be the next step? Do we need to re-attempt the fix for this issue, or do we need to test to see if the issue was resolved with the upstream update? |
Lets test to see if the issue has been resolved. It looks fixed to me on Android, I'll test my iOS device in a few hours once it's charged. |
I forgot about payment, I think we can ignore the 7 day hold |
This issue has not been updated in over 15 days. @eVoloshchak, @Julesssss, @sonialiap, @QichenZhu 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! |
@sonialiap I think we are good to pay out |
Thanks @Julesssss! |
Payment summary:
|
$250 approved for @eVoloshchak |
If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!
Version Number: 1.4.71-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: N/A
Issue reported by: Applause - Internal Team
Action Performed:
Expected Result:
The Next button is fully visible
Actual Result:
Only the top of the Next button is visible. It is reproducible with Manual and Scan tabs
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
Add any screenshot/video evidence
Bug6474132_1715107077940.IMG_6781.mp4
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @eVoloshchakThe text was updated successfully, but these errors were encountered: