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 2022-12-07] [Dark mode] Checkboxes always look checked in dark mode #12910

Closed
aldo-expensify opened this issue Nov 22, 2022 · 19 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Engineering Internal Requires API changes or must be handled by Expensify staff Weekly KSv2

Comments

@aldo-expensify
Copy link
Contributor

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:

  1. Start adding a VBA, get to the company step
  2. The checkbox at the bottom about restricted industries always look checked

Expected Result:

When the checkbox is not checked, it shouldn't have a check. Also, the blue background doesn't seem correct either

Actual Result:

The checkbox always have a check. I one of the states, it has a blueish background.

Screen.Recording.2022-11-21.at.4.16.33.PM.mov

Workaround:

By brute force you can find out which state is the right one to continue

Platform:

Where is this issue occurring?

  • Web
  • iOS
  • Android
  • Desktop App
  • Mobile Web

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

@aldo-expensify aldo-expensify added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Nov 22, 2022
@melvin-bot
Copy link

melvin-bot bot commented Nov 22, 2022

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

@aldo-expensify
Copy link
Contributor Author

cc @grgia

@grgia
Copy link
Contributor

grgia commented Nov 22, 2022

cc @shawnborton do we have a mockup with a checkmark?

@grgia
Copy link
Contributor

grgia commented Nov 22, 2022

I'm happy to fix this one if so!

Here's the old checkbox for reference
https://user-images.githubusercontent.com/38015950/203185404-4cd748a3-a2c8-4d6f-854f-af5774dce520.mov
image
image

@grgia grgia assigned grgia and unassigned greg-schroeder Nov 22, 2022
@shawnborton
Copy link
Contributor

Whoa - strange that a checkmark appears even when it is not checked. Is there a way to just get rid of that white inner checkmark when this is not in the checked state?

@grgia
Copy link
Contributor

grgia commented Nov 22, 2022

yep! besides removing the checkmark, is the blue outline/fill okay?

@shawnborton
Copy link
Contributor

If we're able to change that to our brand green (both outline and fill), that would be great!

@shawnborton
Copy link
Contributor

Also @grgia just wanted to make you aware of this issue in case it's related at all: #11283

@grgia
Copy link
Contributor

grgia commented Nov 22, 2022

Screen.Recording.2022-11-22.at.12.43.17.PM.mov

How do we feel about this?

@grgia grgia mentioned this issue Nov 22, 2022
100 tasks
@grgia grgia added the Internal Requires API changes or must be handled by Expensify staff label Nov 22, 2022
@shawnborton
Copy link
Contributor

That works for me. Is it easy to change that color to green though? I know for CSS, we can do something neat like this:

input[type="checkbox"] {
    accent-color: desired-color
}

@grgia
Copy link
Contributor

grgia commented Nov 22, 2022

Yep! Do you want to change the green to a different color?

@aldo-expensify
Copy link
Contributor Author

Yep! Do you want to change the green to a different color?

I think he means the blueish background to green :P

@shawnborton
Copy link
Contributor

Yup, that's what I mean! So we keep the green outline, and then ideally the check fill is green too.

@grgia
Copy link
Contributor

grgia commented Nov 22, 2022

Screen.Recording.2022-11-22.at.3.53.26.PM.mov

How's this?

@shawnborton
Copy link
Contributor

Perfect!!

@melvin-bot melvin-bot bot added the Overdue label Nov 25, 2022
@melvin-bot
Copy link

melvin-bot bot commented Nov 28, 2022

@grgia Huh... This is 4 days overdue. Who can take care of this?

@grgia
Copy link
Contributor

grgia commented Nov 28, 2022

Should be fixed with dark mode merge. (currently in DEV)

@grgia grgia closed this as completed Nov 28, 2022
@melvin-bot melvin-bot bot added Weekly KSv2 and removed Overdue Daily KSv2 labels Nov 28, 2022
@melvin-bot melvin-bot bot added the Awaiting Payment Auto-added when associated PR is deployed to production label Nov 30, 2022
@melvin-bot
Copy link

melvin-bot bot commented Nov 30, 2022

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.2.33-7 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-07. 🎊

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 melvin-bot bot changed the title [Dark mode] Checkboxes always look checked in dark mode [HOLD for payment 2022-12-07] [Dark mode] Checkboxes always look checked in dark mode Nov 30, 2022
@melvin-bot
Copy link

melvin-bot bot commented Nov 30, 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:

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

No branches or pull requests

4 participants