getAlby / lightning-browser-extension

The Bitcoin Lightning Browser Extension that brings deep Lightning & Nostr integration to the web. Wallet interface to multiple lightning nodes and key signer for Nostr, Liquid and onchain use.
https://getalby.com/#extension
MIT License
539 stars 194 forks source link

UI - dark mode: Websites settings menu focus colors/text not enough contrast #656

Closed escapedcat closed 2 years ago

escapedcat commented 2 years ago

Describe the bug Problem:

image

To Reproduce Steps to reproduce the behavior:

  1. Visit Websites
  2. Click on Alby
  3. Use tab to focus on settings icon & press enter (all with keyboard)
  4. "Edit" is higlighted in menu but it's white text on light-grey background

Expected behavior Good contrast

Device Information [optional]:

Are you working on this issue? No

Related issues

655

bumi commented 2 years ago

thanks, @secondl1ght (head of dark-mode :D) can you look into this?

secondl1ght commented 2 years ago

Thanks for reporting, will put in a fix for this soon!

secondl1ght commented 2 years ago

I just checked it out with the latest version and I cannot reproduce, are you on the latest version?

image

escapedcat commented 2 years ago

Using the keyboard navigation I can still reproduce this issue.

See: keyboard-issue

Do you see this as well?

secondl1ght commented 2 years ago

Hmm ok yeah now I see it.. that is a weird edge case. I dont think many people use their keyboard to navigate the app, I looked into it and it seems like the hover: class does not work for keyboards. Some people recommended substituting focus: for this to work similar on keyboards but it only works on some elements. Maybe @dylancom you have another idea on how to solve for this? Or maybe it is so rare that we don't need to, @bumi what do you think?

escapedcat commented 2 years ago

Hmm ok yeah now I see it.. that is a weird edge case. I dont think many people use their keyboard to navigate the app,

Yeah, it's not a high prio.
But apart from people relying on keyboard-navigation there are tons of people out there who prefer keyboard navigation. Bitcoin might be their bubble.

I looked into it and it seems like the hover: class does not work for keyboards. Some people recommended substituting focus: for this to work similar on keyboards but it only works on some elements.

You have links to these findings? If this is i.e. a Tailwind issue they might already have an issue for this we could link.

dylancom commented 2 years ago

Here you can find how to style the active (focussed) item of a Menu. https://headlessui.dev/react/menu#styling-the-active-item

secondl1ght commented 2 years ago

Nice @dylancom, @escapedcat do you want to do the fix for this? - just checking since you put in the issue.

escapedcat commented 2 years ago

you want to do the fix for this? - just checking since you put in the issue

Just wanted to document it first really. Go ahead if you are interested. Otherwise it could be a "good first issue" as well maybe.

bumi commented 2 years ago

I think this is fixed?

escapedcat commented 2 years ago

Yup!