search

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
2.99k stars 2.5k forks source link

[HOLD for payment 2024-05-20] [$250] Skeleton loader uses lighter color for page header #40244

Open m-natarajan opened 1 month ago

m-natarajan commented 1 month 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: 1.4.62-4 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/p1713166267469379

Action Performed:

  1. Open a chat which will show a skeleton loader
  2. Look at the header of the chat report

Expected Result:

The skeleton loader for the top page/chat header should use the same color background as the skeleton loader for the chat messages in the main content screen.

Actual Result:

The skeleton loader used in the top page/chat header is using slightly lighter colors

Workaround:

unknown

Platforms:

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

Screenshots/Videos

Add any screenshot/video evidence

CleanShot 2024-04-15 at 09 29 33@2x Untitled

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0199d049e1997c5cb7
  • Upwork Job ID: 1779899140122308608
  • Last Price Increase: 2024-04-15
  • Automatic offers:
    • abdulrahuman5196 | Reviewer | 0
    • GandalfGwaihir | Contributor | 0
Issue OwnerCurrent Issue Owner: @twisterdotcom
melvin-bot[bot] commented 1 month ago

Triggered auto assignment to @twisterdotcom (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

GandalfGwaihir commented 1 month ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

Loading lines colour of header and report doesn't match

What is the root cause of that problem?

Currently we have the lines colour as theme.highlightBG this gives us faint lines https://github.com/Expensify/App/blob/1cb6e00a5ffbf120accaaae6deab88290bcc0bd7/src/components/ReportHeaderSkeletonView.tsx#L47-L53

What changes do you think we should make in order to solve the problem?

We need to use skeletonLHNIn as bg colour and theme.skeletonLHNOut as foreground colour.

We do the same for report screen: https://github.com/Expensify/App/blob/1cb6e00a5ffbf120accaaae6deab88290bcc0bd7/src/components/ReportActionsSkeletonView/SkeletonViewLines.tsx#L18-L24

The colour of these styles for dark mode is :

https://github.com/Expensify/App/blob/1cb6e00a5ffbf120accaaae6deab88290bcc0bd7/src/styles/theme/themes/dark.ts#L85-L86

And for light mode is:

https://github.com/Expensify/App/blob/1cb6e00a5ffbf120accaaae6deab88290bcc0bd7/src/styles/theme/themes/light.ts#L85-L86

This should make both the report as well as the report header consistent in loading colour

What alternative solutions did you explore? (Optional)

twisterdotcom commented 1 month ago

Ahh:

Issue reported by: @shawnborton

Gonna make external.

melvin-bot[bot] commented 1 month ago

Job added to Upwork: https://www.upwork.com/jobs/~0199d049e1997c5cb7

melvin-bot[bot] commented 1 month ago

Triggered auto assignment to Contributor-plus team member for initial proposal review - @abdulrahuman5196 (External)

mananjadhav commented 1 month ago

I was too slow in posting a proposal. 🐌

GandalfGwaihir commented 1 month ago

I was too slow in posting a proposal. 🐌

😆 godspeed

dragnoir commented 1 month ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

Skeleton loader uses lighter color for page header.

What is the root cause of that problem?

On the hedare we are using ReportHeaderSkeletonView for the loading animation But on the report we use ReportActionsSkeletonView

Those boath components uses the same SkeletonViewContentLoader with different parameters here: https://github.com/Expensify/App/blob/fdae0bd2ffed0d14cb60633a8d6f7f40c412e170/src/components/ReportHeaderSkeletonView.tsx#L47-L52

and here: https://github.com/Expensify/App/blob/fdae0bd2ffed0d14cb60633a8d6f7f40c412e170/src/components/ReportActionsSkeletonView/SkeletonViewLines.tsx#L18-L23

Also there's an issue with the alignment of the animation

image

What changes do you think we should make in order to solve the problem?

1- to fix the colors: we shoudl use the same backgroundColor and foregroundColor

backgroundColor={theme.skeletonLHNIn}
foregroundColor={theme.skeletonLHNOut}

2- align the header animation same as report animation we should remove the padding left here https://github.com/Expensify/App/blob/fdae0bd2ffed0d14cb60633a8d6f7f40c412e170/src/components/ReportHeaderSkeletonView.tsx#L33

POC: image

dragnoir commented 1 month ago

@shawnborton what devices did you use for the screenshot?

I noticed a bad alignment (more padding on the left) on the header skeleton on web

image

AbrarAlHasan commented 1 month ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

Inconsistent Color in Skeleton Loader header between the header and Main Content Screen

What is the root cause of that problem?

The Root cause of this Problem is due to the wrong Color used in the SkeltonViewContentLoader

What changes do you think we should make in order to solve the problem?

Changing the Color in the Skeleton Header to the color used in the Main content Screen will Solve this Problem.

What alternative solutions did you explore? (Optional)

melvin-bot[bot] commented 1 month ago

📣 @AbrarAlHasan! 📣 Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

  1. Make sure you've read and understood the contributing guidelines.
  2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details. Screen Shot 2022-11-16 at 4 42 54 PM Format: 
    Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>
AbrarAlHasan commented 1 month ago

Contributor details Your Expensify account email: abraralhasan111@gmail.com Upwork Profile Link: https://www.upwork.com/freelancers/~01d42425e0d283e140

melvin-bot[bot] commented 1 month ago

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

shawnborton commented 1 month ago

I noticed a bad alignment (more padding on the left) on the header skeleton on web

@dragnoir this was on desktop. I agree, we should fix the alignment too. cc @twisterdotcom - I like @dragnoir 's proposal to fix both the colors and alignment.

melvin-bot[bot] commented 1 month ago

📣 @abdulrahuman5196 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job

melvin-bot[bot] commented 1 month ago

📣 @dragnoir You have been assigned to this job! Please apply to the Upwork job and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Once you apply to this job, your Upwork ID will be stored and you will be automatically hired for future jobs! Keep in mind: Code of Conduct | Contributing 📖

GandalfGwaihir commented 1 month ago

@twisterdotcom , my main solution is still being used over here, shouldn’t i also be eligible for compensation here?

twisterdotcom commented 1 month ago

We could split this then @dragnoir? Given you're doing the work, how do we feel about 70/30?

dragnoir commented 1 month ago

@twisterdotcom perfect, thank you

GandalfGwaihir commented 1 month ago

@twisterdotcom , @shawnborton wait, there are two points here, the 2nd issue which @dragnoir mentioned doesn't exist, can you share the reproducible steps for the bug you identified @dragnoir :

https://github.com/Expensify/App/assets/110545952/5f1bd83f-5c3f-44d2-b906-1321e76ee935

Secondly, if we implement the selected proposal to remove the padding altogether then there will be an regression for desktop app:

https://github.com/Expensify/App/assets/110545952/c2447bcd-b825-4ad9-b94c-28e84aedf7fc

dragnoir commented 1 month ago

@GandalfGwaihir adjust the isLoading state for HeaderView if you want to test.

https://github.com/Expensify/App/assets/12425932/7a04e515-d566-44eb-9a8c-a13e9f3c5db5

Don't worry, I will make sure no regressions on the PR.

GandalfGwaihir commented 1 month ago

adjust the isLoading state for HeaderView if you want to test.

Still not reproducible @dragnoir , I don't think so the bug you mentioned exists :) @twisterdotcom @shawnborton

https://github.com/Expensify/App/assets/110545952/51585fda-2e46-4272-9317-6b77c77dec06

dragnoir commented 1 month ago

@GandalfGwaihir just change this line https://github.com/Expensify/App/blob/c75c85f69d46a50258b0cb2870b7b5a4d04702cc/src/pages/home/HeaderView.tsx#L215

to !isLoading

then you can always see the ReportHeaderSkeletonView whatever the conditions.

If you want a real reproduction, you can play with network requests to inject js in the console.

Let me know if you see it now.

GandalfGwaihir commented 1 month ago

to !isLoading

then you can always see the ReportHeaderSkeletonView whatever the conditions.

You are assuming it wrong, the report is never in loading state,

And if you change that condition , then you have to also update the condition above it, because you can't assume same condition to be true as well as false at the same time :) https://github.com/Expensify/App/blob/c75c85f69d46a50258b0cb2870b7b5a4d04702cc/src/pages/home/HeaderView.tsx#L214

@twisterdotcom @shawnborton , they assumed the wrong condition and hence they saw that bug, in reality the bug doesn't exist as isLoading is synchronous across, the bug doesn't exist in our current codebase :)

dragnoir commented 1 month ago

@GandalfGwaihir changing isLoading was just for you to understand the idea.

I am AFK now, I will explain better for you later.

GandalfGwaihir commented 1 month ago

@twisterdotcom @shawnborton can you please address these comments when you find time 😄

The other bug pointed out by other contributor doesn’t exist :)

shawnborton commented 1 month ago

If the bug pointed out doesn't exist, then let's ignore it and just address the original bug report from this issue.

GandalfGwaihir commented 1 month ago

Then can i be assigned to the issue? as i was the first one to propose the solution @twisterdotcom @shawnborton

twisterdotcom commented 1 month ago

Let's wait for @dragnoir - it sounds like they did think it exists for some reason. If they confirm it doesn't and we all agree, then of course, I think we can switch it out for you @GandalfGwaihir.

GandalfGwaihir commented 1 month ago

Yeah cool, @dragnoir if the reported bug still exists, can you please attach a video of Production/Staging (Not Dev) environment along with steps to reproduce the bug you reported, thanks :)

dragnoir commented 1 month ago

@GandalfGwaihir No I can't reproduce this on Production/Staging, I need to hard-code it for testing the colors. Like this or the examples below.

@twisterdotcom @shawnborton yes it's an edge case. I am OK to assign @GandalfGwaihir .And sorry about that. I just try to make things perfect.

BTW @shawnborton the light color for the skeleton, while I am testing, I found it's used on other areas like:

What about using the same colors in all loading skeletons?

shawnborton commented 1 month ago

I think this place should also be fixed: CleanShot 2024-04-17 at 19 20 00@2x

But the money request preview component should not be changed.

GandalfGwaihir commented 1 month ago

I think this place should also be fixed:

Cool, can you assign me here @shawnborton so that i can come up with a PR?

shawnborton commented 1 month ago

@twisterdotcom is managing the issue on our end, so I will let him take care of that. I'm just here for design guidance :)

GandalfGwaihir commented 1 month ago

friendly bump to @twisterdotcom for assignment as per @dragnoir's comments

twisterdotcom commented 1 month ago

Yes, happy to assign you. I know we've sort of circumvented @abdulrahuman5196 a bit but happy with Shawn's take from Design over an Internal engineer.

melvin-bot[bot] commented 1 month ago

📣 @GandalfGwaihir 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

GandalfGwaihir commented 3 weeks ago

PR ready for review @abdulrahuman5196

abdulrahuman5196 commented 3 weeks ago

Hi, Thank you. Will work on review.

GandalfGwaihir commented 3 weeks ago

friendly bump to @abdulrahuman5196 for review :)

GandalfGwaihir commented 2 weeks ago

friendly bump @abdulrahuman5196 for review, let's get this merged soon 🚀

abdulrahuman5196 commented 2 weeks ago

Hi, Checking the PR now.

melvin-bot[bot] commented 2 weeks ago

Triggered auto assignment to @marcochavezf, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

GandalfGwaihir commented 2 weeks ago

Friendly bump for PR review @marcaaron

GandalfGwaihir commented 1 week ago

friendly bump @marcochavezf for PR review

melvin-bot[bot] commented 5 days ago

Reviewing label has been removed, please complete the "BugZero Checklist".

melvin-bot[bot] commented 5 days ago

The solution for this issue has been :rocket: deployed to production :rocket: in version 1.4.72-1 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2024-05-20. :confetti_ball:

For reference, here are some details about the assignees on this issue:

melvin-bot[bot] commented 5 days ago

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: