-
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
[WAITING ON PR MERGE] Desktop - Margin above Request Money chat report header #21503
Comments
Triggered auto assignment to @jliexpensify ( |
Bug0 Triage Checklist (Main S/O)
|
Please re-state the problem that we are trying to solve in this issue.Header background color differs for IOU report in desktop. What is the root cause of that problem?HeaderGap is applied to ScreenWrapper component and there we don't have any info about the report type. This issue exists for desktop and for moneyRequest report where header bg is not the same as chat report type. What changes do you think we should make in order to solve the problem?We need to pass an extra prop called headerGapBackground for ScreenWrapper from ReportScreen here and pass the respective green color when the report type is iou. App/src/pages/home/ReportScreen.js Line 250 in 6728589
This prop we need to maintain and pass to HeaderGap component from Screenwrapper to apply dynamic bg color.App/src/components/ScreenWrapper/index.js Line 134 in 6728589
Ref - #19433 (comment) |
I believe this was intentionally added and kept that way since we are aligning with LHN cc @shawnborton |
I can't repro this on v1.3.32-5 even after logging out/in again: @allroundexperts what version are you on? It also seems odd that this is intentional, it's a bit jarring - but I'll wait for @shawnborton to weigh in before I close this GH. |
@jliexpensify I tested it by running the desktop app locally on |
Oh, so my mistake: I didn't realise this was the Desktop version! I re-downloaded it (I've not been using Desktop) and can see it on v1.3.31-3: @shawnborton - before I apply the External label, can you confirm if this is intentional? |
I agree that the top space is intentional, but it should match the color below it. That being said, we're redoing all of the headers for money requests/tasks/etc, so I think we could probably close this since changes are coming. Also, I could have sworn we reported this one like weeks ago as well? cc @JmillsExpensify @trjExpensify @grgia @thienlnam @jasperhuangg |
Yes, this was implemented in the way as the task header so they have the same problem - though this problem shouldn't occur anymore once #20844 we add this, so let's just wait for this to be done instead of trying to solve this |
Still on hold |
Still on HOLD |
Linked PR was merged! |
@allroundexperts since you qualify for a reporting bonus, do you want to be paid via New.Dot? |
Yes. I'll send a request. Thanks @jliexpensify! |
@jliexpensify the issue is for money request report which still exists due to diff background color. |
@Pujan92 looks like you're on Dev, and I'm on Production Desktop. @thienlnam do you mind having a look at this issue? |
@jliexpensify this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks! |
Nice, thanks @grgia ! |
@jliexpensify this issue is now 4 weeks old and preventing us from maintaining WAQ. This should now be your highest priority. Please post below what your plan is to get a PR in review ASAP. Thanks! |
Job added to Upwork: https://www.upwork.com/jobs/~01b8eeb668f4618a9e |
Triggered auto assignment to Contributor Plus for review of internal employee PR - @abdulrahuman5196 ( |
Looks like these were deployed to prod over the weekend, will re-test now. |
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:
The chat header should not contain a top margin
Actual Result:
The chat header contains a top margin
Workaround:
UNknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Version Number: n/a
**Reproducible in staging?:**n/a
Reproducible in production?: v1.3.31-3 (desktop)
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
Notes/Photos/Videos: Any additional supporting documentation
Expensify/Expensify Issue URL:
Issue reported by: Sibtain Ali
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1687147774347499
View all open jobs on GitHub
Upwork Automation - Do Not Edit
The text was updated successfully, but these errors were encountered: