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-20] [$1000] (Dark Theme) Tab highlight color is not correct. #13256

Closed
kavimuru opened this issue Dec 2, 2022 · 42 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2

Comments

@kavimuru
Copy link

kavimuru commented Dec 2, 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:

  1. Open the app on web.
  2. Click on the search icon on LHN.
  3. Use Escape key to close the page.
  4. Press Tab multiple times until first item on the LHN is highlighted.

Expected Result:

The Highlight outline should be #5AB0FF, and ensure we use this regardless of dark or light mode.

Actual Result:

The highlight outline is blue which is part of light theme and should match the dark theme

Workaround:

unknown

Platform:

Where is this issue occurring?

  • Web
  • Desktop App

Version Number: 1.2.35-0
Reproducible in staging?: y
Reproducible in production?: y
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos:
image

Recording.1038.mp4

Expensify/Expensify Issue URL:
Issue reported by: @parasharrajat
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1669646862849829

View all open jobs on GitHub

Upwork Automation - Do Not Edit

@kavimuru kavimuru added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Dec 2, 2022
@melvin-bot
Copy link

melvin-bot bot commented Dec 2, 2022

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

@dhairyasenjaliya
Copy link
Contributor

dhairyasenjaliya commented Dec 2, 2022

Proposal

  • We need to change the color on the below file to focus-visible & focus[data-focusvisible-polyfill] property box-shadow to greenHover: '#00C271',

  • This greenHover color looks neat & clean and but might we need to confirm the color with the design team

App/web/index.html

Lines 35 to 42 in a8e3cb5

:focus-visible {
outline: 0;
box-shadow: inset 0px 0px 0px 1px #0185ff;
}
:focus[data-focusvisible-polyfill] {
outline: 0;
box-shadow: inset 0px 0px 0px 1px #0185ff;
}

        :focus-visible {
            outline: 0;
-            box-shadow: inset 0px 0px 0px 1px #0185ff;
+            box-shadow: inset 0px 0px 0px 1px #00C271;
        }
        :focus[data-focusvisible-polyfill] {
            outline: 0;
-            box-shadow: inset 0px 0px 0px 1px #0185ff;
+            box-shadow: inset 0px 0px 0px 1px #00C271;
        }

Result
Screenshot 2022-12-02 at 11 49 29 AM

@sketchydroide
Copy link
Contributor

picking this up myself

@sketchydroide sketchydroide self-assigned this Dec 2, 2022
@parasharrajat
Copy link
Member

I would suggest discussing this first with Design team to decide if this is really needed. I had doubt on this as I posted on slack.

Note: Please discuss this issue if we really need to change the outline color.

@sketchydroide
Copy link
Contributor

@Expensify/design any input here?

@michaelhaxhiu
Copy link
Contributor

michaelhaxhiu commented Dec 2, 2022

Hm I'm not convinced this is a bug as it stands.

The Expected Result section needs to be more specific too, as right now it's open ended:

Expected Result:

The Highlight outline should match the dark theme.

What would matching the dark theme actually mean?

@parasharrajat
Copy link
Member

What would matching the dark theme actually mean?

This is where the design team kicks in.

@michaelhaxhiu
Copy link
Contributor

Understood. I'm not going to export this job to Upwork until we define/agree on the Expected Result.

https://expensify.slack.com/archives/C049HHMV9SM/p1669986622516479?thread_ts=1669646862.849829&cid=C049HHMV9SM

@shawnborton
Copy link
Contributor

Cool, commenting over in Slack.

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

seem like @shawnborton has decided on a blue color, the one from the links on the app, which I think it's still different from this one. So this GH still makes some sense

@melvin-bot melvin-bot bot removed the Overdue label Dec 5, 2022
@michaelhaxhiu
Copy link
Contributor

Can we delineate which blue we have now, vs the blue we want to use? I want to make sure the expected solution is accurate before we work on this further.

Also - @sketchydroide do you agree this is still external.

@michaelhaxhiu michaelhaxhiu added the External Added to denote the issue can be worked on by a contributor label Dec 5, 2022
@melvin-bot
Copy link

melvin-bot bot commented Dec 5, 2022

Current assignee @michaelhaxhiu is eligible for the External assigner, not assigning anyone new.

@melvin-bot melvin-bot bot changed the title (Dark Theme) Tab highlight color is not correct. [$1000] (Dark Theme) Tab highlight color is not correct. Dec 5, 2022
@melvin-bot
Copy link

melvin-bot bot commented Dec 5, 2022

Job added to Upwork: https://www.upwork.com/jobs/~015b64bc84249fb93c

@melvin-bot
Copy link

melvin-bot bot commented Dec 5, 2022

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Dec 5, 2022
@melvin-bot
Copy link

melvin-bot bot commented Dec 5, 2022

Current assignee @sketchydroide is eligible for the External assigner, not assigning anyone new.

@michaelhaxhiu
Copy link
Contributor

While this is externally resolvable, @sketchydroide is electing to work on it and self assigned for that reason. C+ is assigned for the PR review.

@michaelhaxhiu michaelhaxhiu removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Dec 5, 2022
@sketchydroide
Copy link
Contributor

the color would be blueLink: '#5AB0FF',

@michaelhaxhiu
Copy link
Contributor

quick / stupid question - do we have light mode anymore? I don't see it in my preferences/settings. I don't miss it yet 😂 but just asking for sake up updating GH

@aimane-chnaif
Copy link
Contributor

quick / stupid question - do we have light mode anymore? I don't see it in my preferences/settings. I don't miss it yet 😂 but just asking for sake up updating GH

We're using only dark theme for now. Switching between light an dark theme will be worked on as a follow up.
https://expensify.slack.com/archives/C01GTK53T8Q/p1669226733259059

@sketchydroide
Copy link
Contributor

I created the PR

@shawnborton
Copy link
Contributor

Should we follow up with a PR to round the corners, and particularly, match the roundness of buttons?

@sketchydroide
Copy link
Contributor

sketchydroide commented Dec 6, 2022

we were just mentioning that on slack https://expensify.slack.com/archives/C049HHMV9SM/p1669646862849829

@sketchydroide
Copy link
Contributor

the PR has been merged

@JmillsExpensify JmillsExpensify added the Reviewing Has a PR in review label Dec 7, 2022
@michaelhaxhiu
Copy link
Contributor

Noice

@sketchydroide
Copy link
Contributor

maybe it will go in the next release 🤞🏼

@michaelhaxhiu
Copy link
Contributor

kewl

@michaelhaxhiu
Copy link
Contributor

nice work moving fast :)

@sketchydroide
Copy link
Contributor

this is in prod, Ithink we can close it, unless we need to pay something, @michaelhaxhiu

@michaelhaxhiu
Copy link
Contributor

Issue reported by: @parasharrajat

$250 for rajat catching & reporting this

@parasharrajat
Copy link
Member

parasharrajat commented Dec 13, 2022

I applied for the job 5 days back. @michaelhaxhiu can you please look into that? https://www.upwork.com/jobs/~01cf116c84fb1d9f8a

@michaelhaxhiu
Copy link
Contributor

Accepted, waiting for you to confirm next. Drop me a comment here when you do.

@aimane-chnaif
Copy link
Contributor

@michaelhaxhiu Also, C+ payment for PR review

@parasharrajat
Copy link
Member

Accepted.

@michaelhaxhiu
Copy link
Contributor

@aimane-chnaif yep, don't worry! :) I will dispatch you payment in 2 days via https://www.upwork.com/jobs/~012a0e934343a83aee

Invited you to that job ^

@aimane-chnaif
Copy link
Contributor

Invited you to that job ^

Accepted. Thanks @michaelhaxhiu

@michaelhaxhiu
Copy link
Contributor

Cool, hired!

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Daily KSv2 labels Dec 13, 2022
@melvin-bot
Copy link

melvin-bot bot commented Dec 13, 2022

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

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 [$1000] (Dark Theme) Tab highlight color is not correct. [HOLD for payment 2022-12-20] [$1000] (Dark Theme) Tab highlight color is not correct. Dec 13, 2022
@melvin-bot
Copy link

melvin-bot bot commented Dec 13, 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:

  • [@aimane-chnaif / @sketchydroide] The PR that introduced the bug has been identified. Link to the PR:
  • [@aimane-chnaif / @sketchydroide] 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:
  • [@aimane-chnaif / @sketchydroide] 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:
  • [@michaelhaxhiu] 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:

@sketchydroide
Copy link
Contributor

I don't think there is an offending PR here, it's jus that the UI changes made this apparent, maybe it was forgotten, IDK

@michaelhaxhiu
Copy link
Contributor

Agreed, this isn't a bug or error in our process. Checking the final box.

@michaelhaxhiu
Copy link
Contributor

All paid.

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. External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2
Projects
None yet
Development

No branches or pull requests

8 participants