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 regression test steps, maybe] [Dark Mode] Use same overlay color for all modals, action sheets, etc #13208

Closed
shawnborton opened this issue Nov 30, 2022 · 33 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Internal Requires API changes or must be handled by Expensify staff Monthly KSv2

Comments

@shawnborton
Copy link
Contributor

shawnborton commented Nov 30, 2022

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:

Currently we have mismatched styles when opening things like rightDocked modals or bottom action sheets:

image

Expected Result:

Let's standardize on the overlay color used for all modals (both web and mobile).
image

Let's use our brand dark green (#002E22) at 60% opacity.

Actual Result:

Styles are mismatched and not using the right colors.

Workaround:

This doesn't block user actions.

Platform:

Where is this issue occurring? All platforms

Version Number:
Reproducible in staging?:
Reproducible in production?:
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:
Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01a2fe253f63b94c3c
  • Upwork Job ID: 1606452536036261888
  • Last Price Increase: 2022-12-24
@shawnborton shawnborton added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Nov 30, 2022
@melvin-bot
Copy link

melvin-bot bot commented Nov 30, 2022

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

@grgia grgia added the Internal Requires API changes or must be handled by Expensify staff label Nov 30, 2022
@grgia
Copy link
Contributor

grgia commented Nov 30, 2022

This will make a big difference, I'm gonna take this on internally this week

@shawnborton
Copy link
Contributor Author

Woo, thank you Georgia!

@shawnborton
Copy link
Contributor Author

shawnborton commented Dec 1, 2022

Noting that @dhairyasenjaliya reported that we missed the overlay color here too:
image

Same applies for starting a group chat:
image

To fix this, I would just use the same overlay color that we’re using for modals, etc but keep the text color as it is

@grgia grgia changed the title Use same overlay color for all modals, action sheets, etc [Dark Mode] Use same overlay color for all modals, action sheets, etc Dec 1, 2022
@sonialiap
Copy link
Contributor

Since Georgia is picking it up, I'll skip the triage and unassign myself 👋

@sonialiap sonialiap added NewFeature Something to build that is a new item. and removed NewFeature Something to build that is a new item. labels Dec 1, 2022
@sonialiap sonialiap removed their assignment Dec 1, 2022
@melvin-bot
Copy link

melvin-bot bot commented Dec 5, 2022

@grgia Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@melvin-bot melvin-bot bot added the Overdue label Dec 5, 2022
@grgia
Copy link
Contributor

grgia commented Dec 5, 2022

Looking at this today!

@grgia
Copy link
Contributor

grgia commented Dec 7, 2022

Screen.Recording.2022-12-06.at.7.35.34.PM.mov

Right now, the overlay colors overlap- what's the ideal solution for this modal? (PS I made it pink for visibility 😄 )

@grgia
Copy link
Contributor

grgia commented Dec 7, 2022

Here's what it looks like with the 60% opacity green:

Screen.Recording.2022-12-06.at.7.40.51.PM.mov

@grgia
Copy link
Contributor

grgia commented Dec 7, 2022

Screenshot 2022-12-06 at 7 46 29 PM

For the workspace overlay, should we use this pattern instead?

image

Avatar overlay:
image

@shawnborton
Copy link
Contributor Author

Whoa, didn't realize we were doing the double overlay there. That being said, I think the 60% green works just fine though - thoughts?

For the workspace/avatar overlay, are the screenshots above using 60% green? Maybe we can use the same color from the little +5 screenshot you shared, but use it at something like 88% opacity so you can slightly see the avatar behind? I wouldn't necessarily refactor the small chat row stack for this, but we could try it for the workspace row and chat empty state?

@grgia
Copy link
Contributor

grgia commented Dec 7, 2022

I agree, once I switched it to green I thought the double overlay looked fine. I'll test the workspace with the '+5' color overlay now!

@grgia
Copy link
Contributor

grgia commented Dec 7, 2022

image

image

@shawnborton
Copy link
Contributor Author

Hmm yeah, I'm not loving that as much as a darker shade (like the overlay color). What do you think?

@grgia
Copy link
Contributor

grgia commented Dec 7, 2022

I agree, it somewhat works for the workspaces, but I prefer the green overlay for the 4 avatars.

Here's a few more options

AppBG at 70%
image

Icons color at 100%
image
Borders color at 100% (works when workspace icons are all green, not sure how this will look when we add in the new colored ones)
image

@shawnborton
Copy link
Contributor Author

I think I am leaning towards just using the same color we use for modal overlays for consistency's sake?

@melvin-bot
Copy link

melvin-bot bot commented Dec 15, 2022

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

If no regressions arise, payment will be issued on 2022-12-22. 🎊

After the hold period, please check if any of the following need payment for this issue, and if so check them off after paying:

  • External issue reporter
  • Contributor that fixed the issue
  • Contributor+ that helped on the issue and/or PR

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

  • Merged PR within 3 business days of assignment - 50% bonus
  • Merged PR more than 9 business days after assignment - 50% penalty

@melvin-bot
Copy link

melvin-bot bot commented Dec 15, 2022

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:

  • [@grgia] The PR that introduced the bug has been identified. Link to the PR:
  • [@grgia] 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:
  • [@grgia] 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:
  • [] A regression test has been added or updated so that the same bug will not reach production again. Link to the GH issue for creating the test here:

@grgia
Copy link
Contributor

grgia commented Dec 16, 2022

Checking off as this was more of a feature/color update than a bug. Regression tests will be bundled with dark mode TestRail tests https://github.com/Expensify/Expensify/issues/247948

@mollfpr
Copy link
Contributor

mollfpr commented Dec 22, 2022

@grgia Could you pull someone from BZ to create the Upwork job for the PR review? Thank you!

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Dec 22, 2022
@mallenexpensify mallenexpensify added Internal Requires API changes or must be handled by Expensify staff and removed Internal Requires API changes or must be handled by Expensify staff labels Dec 24, 2022
@melvin-bot
Copy link

melvin-bot bot commented Dec 24, 2022

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

@melvin-bot
Copy link

melvin-bot bot commented Dec 24, 2022

Current assignee @mollfpr is eligible for the Internal assigner, not assigning anyone new.

@mallenexpensify
Copy link
Contributor

@mollfpr can you please apply to the job and reply here once you have?
https://www.upwork.com/jobs/~01a2fe253f63b94c3c

@mallenexpensify mallenexpensify self-assigned this Dec 24, 2022
@mollfpr
Copy link
Contributor

mollfpr commented Dec 24, 2022

@mallenexpensify Can’t access the job

@dhairyasenjaliya
Copy link
Contributor

@mallenexpensify I have reported a bug on slack https://expensify.slack.com/archives/C049HHMV9SM/p1669898961401989 which we are addressing here as per this comment #13208 (comment) do I need to apply for reporting here?

@mollfpr
Copy link
Contributor

mollfpr commented Dec 26, 2022

Bump @mallenexpensify

@mallenexpensify
Copy link
Contributor

mallenexpensify commented Dec 27, 2022

Hired @mollfpr for $1000 for PR review
Hired @dhairyasenjaliya for $250 for bug reporting (thanks for the ping, I would have missed it cuz it wasn't in the OP)
Both, can you please accept the job and reply here once you have?
https://www.upwork.com/jobs/~01a2fe253f63b94c3c

@mollfpr
Copy link
Contributor

mollfpr commented Dec 27, 2022

Accepted, thanks @mallenexpensify

@dhairyasenjaliya
Copy link
Contributor

accepted, thank you @mallenexpensify

@mallenexpensify mallenexpensify added Weekly KSv2 and removed Daily KSv2 labels Dec 27, 2022
@mallenexpensify
Copy link
Contributor

Paid @mollfpr and @dhairyasenjaliya , leaving open and weekly cuz I want to double check the regression steps before closing

@mallenexpensify mallenexpensify changed the title [HOLD for payment 2022-12-22] [Dark Mode] Use same overlay color for all modals, action sheets, etc [HOLD for regression test steps, maybe] [Dark Mode] Use same overlay color for all modals, action sheets, etc Dec 27, 2022
@melvin-bot melvin-bot bot added Monthly KSv2 and removed Weekly KSv2 labels Jan 19, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jan 19, 2023

This issue has not been updated in over 15 days. @mallenexpensify, @mollfpr, @grgia eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

@mallenexpensify mallenexpensify added Daily KSv2 and removed Reviewing Has a PR in review labels Jan 20, 2023
@mallenexpensify
Copy link
Contributor

I know I'd be responsible for one of these issues that needed regression test steps that wasn't going overdue cuz of the Reviewing label, thanks for the reminder Mel. 🤖

OH.. actually, this is just a dark mode update and I think we're not creating/updating TestRail steps for this, at least not yet. Closing!

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 Internal Requires API changes or must be handled by Expensify staff Monthly KSv2
Projects
None yet
Development

No branches or pull requests

6 participants