Skip to content
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 2024-03-07] Splash logo icon is black instead of green (prod only) #35018

Closed
3 of 6 tasks
isagoico opened this issue Jan 23, 2024 · 47 comments
Closed
3 of 6 tasks
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review

Comments

@isagoico
Copy link

isagoico commented Jan 23, 2024

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: v1.4.29-1
Reproducible in staging?: No
Reproducible in production?: Yes
If this was caught during regression testing, add the test name, ID and link from TestRail: N/A
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: @coleaeason
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1706050599347949

Action Performed:

  1. Reload the app

Expected Result:

Splash icon should be green.

Actual Result:

Splash icon is black.

Workaround:

N/A

Platforms:

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

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

image

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01aac5767909c58b6b
  • Upwork Job ID: 1749942692150059008
  • Last Price Increase: 2024-01-23
@isagoico isagoico added External Added to denote the issue can be worked on by a contributor Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jan 23, 2024
@melvin-bot melvin-bot bot changed the title Splash logo icon is black instead of green (prod only) [$500] Splash logo icon is black instead of green (prod only) Jan 23, 2024
Copy link

melvin-bot bot commented Jan 23, 2024

Job added to Upwork: https://www.upwork.com/jobs/~01aac5767909c58b6b

Copy link

melvin-bot bot commented Jan 23, 2024

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jan 23, 2024
Copy link

melvin-bot bot commented Jan 23, 2024

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

@jeremy-croff
Copy link
Contributor

jeremy-croff commented Jan 24, 2024

This is because of a regression from: #33863

Proposal

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

Page loading screen icon color is wrong

What is the root cause of that problem?

A recent code change removed the default svg fill color to improve to hover state of this reused icon in the settings menu list. It did not cross-reference it's usage in the loading splash screen.

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

Create a seperate SVG with the fill color, to be used for the splash screen.

Keep the existing SVG as is for an improved hover state fill color.

What alternative solutions did you explore? (Optional)

@aswin-s
Copy link
Contributor

aswin-s commented Jan 24, 2024

Proposal

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

Expensify icon shown on splash screen is black in production environment for web platforms

What is the root cause of that problem?

The icon file being used as splash icon is new-expensify.svg as per splashLogo configuration in webpack config.

<%= htmlWebpackPlugin.options.splashLogo %>

splashLogo: fs.readFileSync(path.resolve(__dirname, `../../assets/images/new-expensify${mapEnvToLogoSuffix(envFile)}.svg`), 'utf-8'),

const envToLogoSuffixMap = {
production: '',
staging: '-stg',
dev: '-dev',
adhoc: '-adhoc',
};

Other logos have fill color defined in their styles as st2{fill:#03d47c}. However for production logo fill color is missing in styles. That's why the icon looks black.

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

Add fill:#03d47c to svg style definition of new-expensify.svg.

.st0{fill-rule:evenodd;clip-rule:evenodd; fill:#03d47c}

So update new-expensify.svg with the updated style like below

<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" id="Layer_1" x="0" y="0" version="1.1" viewBox="0 0 81 81" style="enable-background:new 0 0 81 81" xml:space="preserve"><style type="text/css">.st0{fill-rule:evenodd;clip-rule:evenodd; fill:#03d47c}</style><g><defs><rect id="SVGID_1_" width="80" height="80" x=".8"/></defs><clipPath id="SVGID_00000144338341264450383030000002729908262559559608_"><use xlink:href="#SVGID_1_" style="overflow:visible"/></clipPath><g style="clip-path:url(#SVGID_00000144338341264450383030000002729908262559559608_)"><path d="M54.9,28.2v-8.3H26.6v40.3h28.2v-8.3H37v-7.7h14.4v-8.5H37v-7.3H54.9z" class="st0"/><path d="M40.8,6.5C40.8,6.5,40.8,6.5,40.8,6.5c7.1,0,13.7,2.2,19.2,6c1.4,1,3.3,0.9,4.5-0.3l0,0 c1.3-1.3,1.3-3.5-0.3-4.7C57.5,2.8,49.5,0,40.8,0c-22.1,0-40,17.9-40,40c0,8.7,2.8,16.8,7.5,23.3c1.1,1.5,3.3,1.6,4.7,0.3l0,0 c1.2-1.2,1.3-3.1,0.3-4.5c-3.8-5.4-6-12-6-19.2C7.3,21.5,22.2,6.5,40.8,6.5C40.7,6.5,40.8,6.5,40.8,6.5L40.8,6.5z" class="st0"/><path d="M73.7,17.4c-1.1-1.6-3.4-1.7-4.7-0.3l0,0c-1.2,1.2-1.3,3-0.4,4.4c3.5,5.3,5.6,11.7,5.6,18.5 c0,6.8-2.1,13.4-5.8,18.8c-0.9,1.4-0.9,3.3,0.3,4.5l0,0c1.4,1.4,3.6,1.3,4.7-0.3c4.6-6.5,7.3-14.4,7.3-23 C80.8,31.5,78.2,23.8,73.7,17.4z" class="st0"/><path d="M40.8,73.5c-6.8,0-13.2-2.1-18.5-5.6c-1.4-0.9-3.3-0.8-4.4,0.4l0,0c-1.4,1.4-1.3,3.6,0.3,4.7 c6.4,4.4,14.2,7,22.6,7c8.4,0,16.4-2.7,23-7.3c1.6-1.1,1.7-3.3,0.3-4.7l0,0c-1.2-1.2-3.1-1.3-4.5-0.3 C54.2,71.3,47.7,73.5,40.8,73.5z" class="st0"/></g></g></svg>

Result

Before After
new-expensify new-expensify

What alternative solutions did you explore? (Optional)

None (edited)

@dukenv0307
Copy link
Contributor

dukenv0307 commented Jan 24, 2024

Proposal

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

Splash icon is black.

What is the root cause of that problem?

From this PR, we've removed the fill color of the new-expensify.svg icon in order to support Hover/non-hover color, leading to the black icon in splash screen because in splash screen we're using the icon as is.

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

  1. In the <svg tag here, we should set fill="currentColor". This currentColor approach is also already used in the eReceiptIcon.

  2. Then add color to the CSS style of the SVG here. We already use the same way of targeting the svg in the @media above that, just that here we will apply to all web screens so no need for @media.

.splash-logo > svg {
    color: #03d47c;
}

The #03d47c color is the color present before the PR that removed it was merged.

  • When the icon is used inside Hovered components, that fill will be overridden with the hover color so no change there
  • When the icon is used in Splash screen, it will take the color CSS style as currentColor and renders properly

With this solution there's no need for a duplicate svg just for the splash screen.

What alternative solutions did you explore? (Optional)

Instead of using SVG directly for the splash screen, we can use <img instead and set src to the svg path, and set the coloring style to the img.

@jliexpensify
Copy link
Contributor

@abdulrahuman5196 can you confirm this?

This is because of a regression from: #33863

If so, this should be assigned to @thesahindia

@shawnborton
Copy link
Contributor

cc @barttom for the possible regression above ^^

@barttom
Copy link
Contributor

barttom commented Jan 24, 2024

@shawnborton Yup, I think this is regression. I didn't know that this file is used anywhere instead of an app icon.
I've removed the defined color because for icon we need to have a neutral color to provide different states e.g. for hover.
I haven't found any possibility to define color for the splash logo, so I'll create a separate file dedicated to a splash logo.
I'll create PR today

@shawnborton
Copy link
Contributor

Wonderful, thanks!

@dukenv0307
Copy link
Contributor

I haven't found any possibility to define color for the splash logo, so I'll create a separate file dedicated to a splash logo.

@barttom I don't think we need a separate file. Please take a look at my proposal.

possibility to define color for the splash logo

@barttom It addresses your concern here.

Thank you!

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Jan 24, 2024
Copy link

melvin-bot bot commented Jan 25, 2024

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

@thesahindia
Copy link
Member

@dangrous, the PR is ready. It just need your eyes.

@melvin-bot melvin-bot bot removed Help Wanted Apply this label when an issue is open to proposals by contributors labels Jan 26, 2024
@jliexpensify
Copy link
Contributor

Hmm sorry, I am not sure I'm understanding here - wasn't this a regression, as correctly identified by @jeremy-croff ? If so, then the GH issue goes to the Contributor and C+ who worked on the original PR which caused the regression. So why is @dukenv0307 proposing compensation?

@jliexpensify
Copy link
Contributor

Said another way - it's great that your proposal is the fix @dukenv0307, but in the case of a regression, it's on the original PR owners to fix it, right? Which is @barttom

@dangrous
Copy link
Contributor

dangrous commented Feb 6, 2024

ha so this one is particularly confusing - I think these are the discussions:

  • @dukenv0307 proposed a solution for this bug, however the issue was determined to be a regression, so the original authors took it on. The solution they pushed is similar. This solution ended up being reverted as well, so it's moot, but even if it was, I don't think we typically offer compensation (but we appreciate the effort!)
  • @jeremy-croff proposed a solution for this bug, however the issue was determined to be a regression, so the original authors took it on. The solution they pushed was reverted, and the new solution is similar to the one @jeremy-croff proposed. Again, I don't think we typically offer compensation for this case, since it's covered by the original authors.

However, it brings up an interesting more general point - think of the following scenario:

  • Issue exists
  • Contributor A proposes Solution A
  • Contributor B proposes Solution B
  • We go with Contributor A, and merge Solution A
  • Solution A causes a regression and is reverted
  • Contributor A looks into the regression and determines that Solution B would be a better fix and implements it.
  • So, in the end, we're using Contributor B's proposal but Contributor A was the only one involved.

Important: I don't think this is likely to happen very often, and I will stress again, since this issue itself was a regression as well, it doesn't apply here either. But might be interesting to discuss at some point. All in all, no action required here I don't think. Just a thought experiment.

@jeremy-croff
Copy link
Contributor

jeremy-croff commented Feb 6, 2024

ha so this one is particularly confusing - I think these are the discussions:

  • @dukenv0307 proposed a solution for this bug, however the issue was determined to be a regression, so the original authors took it on. The solution they pushed is similar. This solution ended up being reverted as well, so it's moot, but even if it was, I don't think we typically offer compensation (but we appreciate the effort!)
  • @jeremy-croff proposed a solution for this bug, however the issue was determined to be a regression, so the original authors took it on. The solution they pushed was reverted, and the new solution is similar to the one @jeremy-croff proposed. Again, I don't think we typically offer compensation for this case, since it's covered by the original authors.

However, it brings up an interesting more general point - think of the following scenario:

  • Issue exists
  • Contributor A proposes Solution A
  • Contributor B proposes Solution B
  • We go with Contributor A, and merge Solution A
  • Solution A causes a regression and is reverted
  • Contributor A looks into the regression and determines that Solution B would be a better fix and implements it.
  • So, in the end, we're using Contributor B's proposal but Contributor A was the only one involved.

Important: I don't think this is likely to happen very often, and I will stress again, since this issue itself was a regression as well, it doesn't apply here either. But might be interesting to discuss at some point. All in all, no action required here I don't think. Just a thought experiment.

I agree and thanks for reviewing the situation. I also was the contributor to identify it as a regression.
It's a tiny bit painful as a new contributor because this has happened to me twice this week and I have not gotten any proposals accepted yet. I will keep on trying though.
This one was unfortunately more verbose copy paste of my logic:
#35663 (comment) and PR https://github.com/Expensify/App/pull/35872/files#diff-78609be64f9c89b1986ed92266ac4cfd0ad05a5751bee8678d61a1e61822ac39R102
Which is not even what the accepted proposal proposed! 🥲

@jliexpensify
Copy link
Contributor

jliexpensify commented Feb 7, 2024

Thanks for summarising @dangrous - and yes, this is a little complex.

So, in the end, we're using Contributor B's proposal but Contributor A was the only one involved.

Honestly, if I had to make a judgment, I would give part payment (maybe like $250) to @jeremy-croff for:

  1. Identifying the regression (note: we wouldn't pay for this)
  2. Providing the actual working solution when no one else could

As 2 regressions basically halves the payment of the C and C+ down to $125 each.

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Feb 7, 2024
Copy link

melvin-bot bot commented Feb 7, 2024

Payment Summary

Upwork Job

  • Contributor: @barttom is from an agency-contributor and not due payment
  • Reviewer: @thesahindia owed $500 via NewDot

BugZero Checklist (@jliexpensify)

  • I have verified the correct assignees and roles are listed above and updated the neccesary manual offers
  • I have verified that there are no duplicate or incorrect contracts on Upwork for this job (https://www.upwork.com/ab/applicants/1749942692150059008/hired)
  • I have paid out the Upwork contracts or cancelled the ones that are incorrect
  • I have verified the payment summary above is correct

@dangrous
Copy link
Contributor

dangrous commented Feb 7, 2024

That makes sense to me @jliexpensify!

So to update/clarify melvin's comment up there ^ - we will not be handling payment fo either the contributor or reviewer on this issue, as it's a regression (it will be handled on the original issue). We will be giving partial payment to @jeremy-croff for their help and solutions.

But no payment should happen yet since the fix of the fix of the fix is still in progress, despite it now being 7 days after the merge of the PR that supposedly fixed this issue but was reverted.

Is that all correct?

@jliexpensify
Copy link
Contributor

Yep, I think that's fair and thanks for considering my approach @dangrous!

So in summary - for this specific GH:

Upworks Job

@jliexpensify jliexpensify changed the title [HOLD for payment 2024-02-07] [$500] Splash logo icon is black instead of green (prod only) [HOLD for payment 2024-02-??] [$500] Splash logo icon is black instead of green (prod only) Feb 7, 2024
@jliexpensify jliexpensify changed the title [HOLD for payment 2024-02-??] [$500] Splash logo icon is black instead of green (prod only) [HOLD for payment 2024-02-??] Splash logo icon is black instead of green (prod only) Feb 7, 2024
@jeremy-croff
Copy link
Contributor

Yep, I think that's fair and thanks for considering my approach @dangrous!

So in summary - for this specific GH:

Upworks Job

I applied to it, is there anything else I need to do to get it accepted?

@jliexpensify
Copy link
Contributor

Huh, is that Job loading for you @jeremy-croff ? I'm getting timeout errors when trying to view it

@jeremy-croff
Copy link
Contributor

Huh, is that Job loading for you @jeremy-croff ? I'm getting timeout errors when trying to view it

Yeah it is

@jliexpensify
Copy link
Contributor

Weird, must have been temporary! Sent you an offer, just got to wait until the fix hits prod - we pay after 7 days.

@jliexpensify
Copy link
Contributor

Not deployed yet

@dangrous
Copy link
Contributor

fix number 3 has been merged! not yet on staging/prod but we should be able to close this out shortly

@jliexpensify
Copy link
Contributor

Deployed to staging yesterday

@jliexpensify jliexpensify changed the title [HOLD for payment 2024-02-??] Splash logo icon is black instead of green (prod only) [HOLD for payment 2024-03-05] Splash logo icon is black instead of green (prod only) Feb 28, 2024
@jliexpensify
Copy link
Contributor

jliexpensify commented Feb 28, 2024

Deployed to prod, adjusted payment date

Copy link

melvin-bot bot commented Mar 5, 2024

Payment Summary

Upwork Job

  • Contributor: @barttom is from an agency-contributor and not due payment
  • Reviewer: @thesahindia owed $500 via NewDot

BugZero Checklist (@jliexpensify)

  • I have verified the correct assignees and roles are listed above and updated the neccesary manual offers
  • I have verified that there are no duplicate or incorrect contracts on Upwork for this job (https://www.upwork.com/ab/applicants/1749942692150059008/hired)
  • I have paid out the Upwork contracts or cancelled the ones that are incorrect
  • I have verified the payment summary above is correct

1 similar comment
Copy link

melvin-bot bot commented Mar 5, 2024

Payment Summary

Upwork Job

  • Contributor: @barttom is from an agency-contributor and not due payment
  • Reviewer: @thesahindia owed $500 via NewDot

BugZero Checklist (@jliexpensify)

  • I have verified the correct assignees and roles are listed above and updated the neccesary manual offers
  • I have verified that there are no duplicate or incorrect contracts on Upwork for this job (https://www.upwork.com/ab/applicants/1749942692150059008/hired)
  • I have paid out the Upwork contracts or cancelled the ones that are incorrect
  • I have verified the payment summary above is correct

@jliexpensify jliexpensify changed the title [HOLD for payment 2024-03-05] Splash logo icon is black instead of green (prod only) [HOLD for payment 2024-03-07] Splash logo icon is black instead of green (prod only) Mar 5, 2024
@jliexpensify
Copy link
Contributor

The above summary is not correct, here's the correct one to be paid.

Copy link

melvin-bot bot commented Mar 7, 2024

Payment Summary

Upwork Job

  • Contributor: @barttom is from an agency-contributor and not due payment
  • Reviewer: @thesahindia owed $500 via NewDot

BugZero Checklist (@jliexpensify)

  • I have verified the correct assignees and roles are listed above and updated the neccesary manual offers
  • I have verified that there are no duplicate or incorrect contracts on Upwork for this job (https://www.upwork.com/ab/applicants/1749942692150059008/hired)
  • I have paid out the Upwork contracts or cancelled the ones that are incorrect
  • I have verified the payment summary above is correct

@jliexpensify
Copy link
Contributor

Paid, job closed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review
Projects
None yet
Development

No branches or pull requests