App
App copied to clipboard
Expensify
[HOLD for payment 2024-02-??] Splash logo icon is black instead of green (prod only)
If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] 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:
- 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
- [x] Android: mWeb Chrome
- [ ] iOS: Native
- [x] iOS: mWeb Safari
- [x] MacOS: Chrome / Safari
- [ ] MacOS: Desktop
Screenshots/Videos
Upwork Automation - Do Not Edit
- Upwork Job URL: https://www.upwork.com/jobs/~01aac5767909c58b6b
- Upwork Job ID: 1749942692150059008
- Last Price Increase: 2024-01-23
Job added to Upwork: https://www.upwork.com/jobs/~01aac5767909c58b6b
![melvin-bot[bot] avatar](https://avatars.githubusercontent.com/in/179547?v=4 "Published by a pub.dev verified publisher"){kind=small}
Triggered auto assignment to @jliexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.
Triggered auto assignment to Contributor-plus team member for initial proposal review - @abdulrahuman5196 (External)
This is because of a regression from: https://github.com/Expensify/App/pull/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)
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.
https://github.com/Expensify/App/blob/0efabc2b40fe845b13faa94ec508a6693167ede9/web/index.html#L168
https://github.com/Expensify/App/blob/0efabc2b40fe845b13faa94ec508a6693167ede9/config/webpack/webpack.common.js#L68
https://github.com/Expensify/App/blob/0efabc2b40fe845b13faa94ec508a6693167ede9/config/webpack/webpack.common.js#L27-L32
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 |
|---|---|
What alternative solutions did you explore? (Optional)
None (edited)
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?
-
In the
<svgtag here, we should setfill="currentColor". ThiscurrentColorapproach is also already used in the eReceiptIcon. -
Then add
colorto the CSS style of the SVG here. We already use the same way of targeting the svg in the@mediaabove 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
fillwill be overridden with the hover color so no change there - When the icon is used in Splash screen, it will take the
colorCSS style ascurrentColorand 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.
@abdulrahuman5196 can you confirm this?
This is because of a regression from: https://github.com/Expensify/App/pull/33863
If so, this should be assigned to @thesahindia
@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
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!
Triggered auto assignment to @dangrous, see https://stackoverflow.com/c/expensify/questions/7972 for more details.
⚠️ Looks like this issue was linked to a Deploy Blocker here
If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.
If a regression has occurred and you are the assigned CM follow the instructions here.
If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.
Reviewing label has been removed, please complete the "BugZero Checklist".
The solution for this issue has been :rocket: deployed to production :rocket: in version 1.4.34-1 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:
- https://github.com/Expensify/App/pull/35097
If no regressions arise, payment will be issued on 2024-02-07. :confetti_ball:
For reference, here are some details about the assignees on this issue:
- @barttom does not require payment (Contractor)
- @thesahindia requires payment through NewDot Manual Requests
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:
- [ ] [@thesahindia] The PR that introduced the bug has been identified. Link to the PR:
- [ ] [@thesahindia] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
- [ ] [@thesahindia] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
- [ ] [@thesahindia] Determine if we should create a regression test for this bug.
- [ ] [@thesahindia] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
- [ ] [@jliexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:
@jliexpensify Since the fix PR for this GH issue is using my proposal, can you assign me to this GH issue for compensation, thank you!
This fix is reverted, I'll send another soon https://github.com/Expensify/App/issues/35468
@jliexpensify Since the fix PR for this GH issue is using my proposal, can you assign me to this GH issue for compensation, thank you!
Is this common practice?
Looks like I had the first proposal and the correct one.
https://github.com/Expensify/App/issues/35018#issuecomment-1907140745
@jeremy-croff @dukenv0307 I'm not an Engineer so I can't make a judgement here
@dangrous @thesahindia tagging you in for some help here! Should @dukenv0307 be assigned?
I'm actually not fully sure what we do in this situation normally - but at this point it's moot since we ended up reverting that fix anyway. So no need to adjust payment at this point as we're waiting for a fix of the regression of the fix of the regression. Le sigh
I'm actually not fully sure what we do in this situation normally - but at this point it's moot since we ended up reverting that fix anyway. So no need to adjust payment at this point as we're waiting for a fix of the regression of the fix of the regression. Le sigh
The current approach in latest fix https://github.com/Expensify/App/pull/35544 is the one that I suggested. The previous approach from @dukenv0307 is what got reverted.
But reading a request from a more experienced contributor to request payment for having the correct suggestion made me curious about exactly this process.. It seems the final fix will be the one I suggested.
However, the actual bug and solution is so rudimentary that I don't necessarily deserve any credit. I just called it from the start :D
Ah, yeah I had only looked back at @dukenv0307's comment not yours, sorry! I think it is a good question in general (although hopefully it happens pretty rarely). Maybe we can discuss internally @jliexpensify? I agree that in this situation it's less needed, but it'll be good to plan for the future.
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?
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
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.
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: https://github.com/Expensify/App/issues/35663#issuecomment-1925066729 and PR https://github.com/Expensify/App/pull/35872/files#diff-78609be64f9c89b1986ed92266ac4cfd0ad05a5751bee8678d61a1e61822ac39R102 Which is not even what the accepted proposal proposed! 🥲