-
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 2022-11-16] [$500] IPAD: Extra space on the above attachment - reported by @ahmdshrif #10352
Comments
Triggered auto assignment to @RachCHopkins ( |
Triggered auto assignment to @johnmlee101 ( |
Seems like a fairly straightforward fix, doubt it was a regression so marking as external |
Triggered auto assignment to @Christinadobrzyn ( |
Job created in Upwork - Internal job posting - https://www.upwork.com/ab/applicants/1559027292353654784/job-details |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @Santhosh-Sellavel ( |
Triggered auto assignment to @Gonals ( |
PROPOSAL I reproduce the problem. The problem seems to be related only to padding. But if we remove the padding, I haven't tested what kind of problem will occur on other screens. |
Current assignee @shawnborton is eligible for the Design assigner, not assigning anyone new. |
Looks like the PR is up - #12130 @mananjadhav @shawnborton @Gonals where are we with reviewing the PR? Can we get an update on what needs to happen to get this finished? |
@Christinadobrzyn The changes I reviewed, I am currently testing them. I'll take me a day or two to finish testing on all platforms |
C+ Review done on the PR |
BugZero Checklist: The PR fixing this issue has been merged! The following checklist will need to be completed before the issue can be closed:
|
I don't think I would call this a bug. We've always had the padding adjusted this way and then with the discussions above, we've refactored the UI to remove the padding altogether. |
I have no idea how to complete most of that BugZero Checklist - asking here: https://expensify.slack.com/archives/C01SKUP7QR0/p1667981905951189 @mananjadhav would you happen to know:
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 1.2.25-0 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 2022-11-16. 🎊 |
@Christinadobrzyn I can say we never design the AttachmentView for iPad. Hence, the fix was to also refactor the component to remove all the padding. Do you still need the PR? |
Am I eligible for some more compensation for this issue? Since lots of discussions happened and final spec is beyond the original issue. |
@mananjadhav that might be a question for @Gonals |
I don't think we need the PR in this case, since it wasn't really a bug, just a change in design down the line. |
Agreed @Gonals.
Agree here with @aimane-chnaif. @aimane-chnaif Did you get a chance to look at @Christinadobrzyn's comment here |
@Christinadobrzyn I was thinking of $750-$1000. But it would depend on your judgement. Thanks |
Thanks for the extra information @aimane-chnaif I reached out to our team to get some thoughts. I'll follow up as soon as I have more information from them and pay this. |
@Christinadobrzyn do you have any updates for us here? |
Sorry for the delay. Closed the job in Upwork and closing this. I paid Let me know if you have any questions! |
Hey @mananjadhav sorry, I missed your question here #10352 (comment) Yes, can you please send me the PR that caused the issue so I can record it in test rail. |
Please check the response here |
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Action Performed:
Expected Result:
No extra space above the attachment like other platforms.
Actual Result:
There is extra space as shown on the screenshot.
Workaround:
unknown.
Platform:
Where is this issue occurring?
Version Number: 1.1.88-0
Reproducible in staging?: Y
Reproducible in production?: Y
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
Expensify/Expensify Issue URL:
Issue reported by: @ahmdshrif
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1658374757439919
View all open jobs on GitHub
The text was updated successfully, but these errors were encountered: