Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.36k stars 2.78k forks source link

The background color for the statement doesn’t match the app #33978

Closed m-natarajan closed 7 months ago

m-natarajan commented 8 months ago

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?: needs reproduction (don't have statement access) Reproducible in production?: needs reproduction (don't have statement access) 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: @puneetlath Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1704219570242489

Action Performed:

  1. Click on the statement link from concierge chat
  2. Observe the background color

Expected Result:

The background color should match the app

Actual Result:

the header and body have different background colors

Workaround:

unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

Screenshots/Videos

Add any screenshot/video evidence

image (9)

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0148c084d01887606e
  • Upwork Job ID: 1744409181225820160
  • Last Price Increase: 2024-01-08
melvin-bot[bot] commented 8 months ago

Triggered auto assignment to @MitchExpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

melvin-bot[bot] commented 8 months ago

Bug0 Triage Checklist (Main S/O)

MitchExpensify commented 8 months ago

Reproduced!

MitchExpensify commented 8 months ago

Asking whether it should be internal or not here: https://expensify.slack.com/archives/C049HHMV9SM/p1704733013601179?thread_ts=1704219570.242489&cid=C049HHMV9SM

melvin-bot[bot] commented 8 months ago

Job added to Upwork: https://www.upwork.com/jobs/~0148c084d01887606e

melvin-bot[bot] commented 8 months ago

Triggered auto assignment to Contributor Plus for review of internal employee PR - @sobitneupane (Internal)

melvin-bot[bot] commented 8 months ago

Triggered auto assignment to @rlinoz (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

melvin-bot[bot] commented 8 months ago

Triggered auto assignment to @dubielzyk-expensify (Design), see these Stack Overflow questions for more details.

rlinoz commented 8 months ago

Hey @dubielzyk-expensify can you help us here?

We have this statement screen that was firstly designed to be in OldDot I think and we have a problem with the background mismatching the NewDot background color.

I was thinking that maybe we could just apply the new background colors to the old screen, but the dark mode gets pretty bad as per the screenshot attached, so I think we have a few options:

1 - Change the background to the light mode background and wait for a better day to maybe migrate this entire functionality to NewDot - This is the way it has been, but I think the new light mode made it more evident that the background shouldn't be the one it is. This is low effort, but it doesn't look really good in dark mode.

2 - Update the theme in this page specifically to match the dark/light mode, but since this page is in OldDot it would be a whole new implementation of themes. - Medium effort, and maybe we will throw this out soon if we have plans for moving the statement screen to NewDot.

3 - Making this a new issue to move statements to NewDot and all use all our new components (maybe there is a new figma layout for this already?) - This is really high effort and we would probably have to take it to #whatsnext or check if it already planned for some wave.

I would appreciate your thoughts on this.

Dark mode:

Screenshot 2024-01-10 at 12 39 53

Light mode:

Screenshot 2024-01-10 at 12 39 30

Dark mode with ligth background:

Screenshot 2024-01-10 at 12 39 05
puneetlath commented 8 months ago

Interesting considerations @rlinoz! It could be good to raise in #vip-split as that's where I think this is most relevant.

dubielzyk-expensify commented 8 months ago

Thanks for a great write up and reaching out @rlinoz. It's hard for me to comment as I'm unsure how severe and common this screen is and how much investment we'd wanna put into it.

My gut reaction is that we should at least go with option 2 as option 1 breaks a lot of our UI guidelines and also the fact that the user has potentially selected dark mode.

While 3 would be the best option, I definitely think 2 feels like the right balance. Keen on @Expensify/design opinion on this as well

shawnborton commented 8 months ago

So I think we really just need to do a better job of making sure the statement view is using all of our standard CSS vars. Because OldDot is already fully ready for dark mode. Just go to www.expensify.com and type in $('body').toggleClass('dark'); in the console

So that being said, once everything in the statement view uses the proper CSS vars, then I think we will be in a much better place. From here, I think it's really just a matter of being able to pass in some kind of url param into the statement url like ?theme=dark or something which would add the body class of dark when needed.

rlinoz commented 8 months ago

I had no idea OldDot was ready for dark mode, that makes things way easier.

I guess statements is already using the correct CSS vars for colors, except for background which I had to change.

Does this looks correct?

image Screenshot 2024-01-11 at 13 00 59
shawnborton commented 8 months ago

That looks correct! While you are here though... something about the font doesn't quite look right. Notice how it feels squished? Any ideas? CleanShot 2024-01-11 at 19 44 23@2x

shawnborton commented 8 months ago

Also, the split bill icon is incorrect - it should be a receipt or arrows going left & right?

rlinoz commented 8 months ago

@shawnborton I couldn't find a left & right icon in our icons, but there is a receipt if that is ok.

About the font, we were using the GT-America-Standard I think we don't use that anymore? Anyway, I changed it to the one we are using in other pages.

image
shawnborton commented 8 months ago

Nice! I think we're getting close. Can you double check that the font is correct though? For example, this part here: CleanShot 2024-01-11 at 20 13 44@2x

If this was our correct font, we'd see a "double-story" g, like so: CleanShot 2024-01-11 at 20 14 28@2x

So I think this is the case for all of the type I see - it's not quite using our actual font.

rlinoz commented 8 months ago

@shawnborton is this better? Should we change font sizes?

image
shawnborton commented 8 months ago

That is definitely not correct, that looks like a default serif font.

rlinoz commented 8 months ago

Sorry about that, I think chrome was not playing nice and I have a hard time discerning fonts, but I think this one is correct

Here is the font order being used "Expensify-Neue", system-ui, "Segoe UI", Roboto, Helvetica, Arial, sans-serif, "Apple Color Emoji", "Segoe UI Emoji", "Segoe UI Symbol"

Screenshot 2024-01-11 at 17 32 02 Screenshot 2024-01-11 at 17 29 11
shawnborton commented 8 months ago

Much better!

rlinoz commented 8 months ago

I forgot to add $ to the PR fixing this issue, and I am not sure how to trigger the payment automation for the C+ review.

Can you help us here @MitchExpensify, please?

MitchExpensify commented 8 months ago

Payment summary:

Please make the request in NewDot for compensation on Jan 24th, @sobitneupane - Thank you!

melvin-bot[bot] commented 8 months ago

@rlinoz @sobitneupane @MitchExpensify @dubielzyk-expensify 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!

rlinoz commented 8 months ago

I think we are just missing @sobitneupane make the request in NewDot?

MitchExpensify commented 7 months ago

Yes and this can be closed while we wait on that

JmillsExpensify commented 7 months ago

$500 approved for @sobitneupane based on this summary.