-
Notifications
You must be signed in to change notification settings - Fork 414
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
Replacing existing colors with npm module, and refactored Sass #2045
Conversation
This is looking great. I really like the new font. I found a few issues and also have a few more generic comments.
|
I'm not super sure about the new green color. I like the actual color but it's a lot harder to read buttons with white text. The contrast ratio is significantly lower than the previous combination which was already pretty low. |
@seanyesmunt Pushed up some changes. The
|
@NetOperatorWibby I don't think we have to pass all, |
The search page is looking better on dark mode. There are still some issues with placeholders. The input background color is also a little tough to read on dark mode (both search and copyable inputs) |
Alright, fixed inputs. Found a rather interesting bug/feature. When you highlight text and then switch themes, the selected text is white and will not fix itself unless you reload the app. |
This is another good site for contrast testing: https://colorable.jxnblk.com/ffffff/2f9176 |
A few last issues (I think) The startup screen/snackbar/text highlighting should use |
1 similar comment
A few last issues (I think) The startup screen/snackbar/text highlighting should use |
color: var(--btn-external-color); | ||
} | ||
&.btn--disabled:disabled { | ||
// wtf? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not super sure if we need this but I think that was for specificity
@NetOperatorWibby After looking at some of the code more closely I'm starting to think we should keep the Currently in some cases, if any layout changes, the dark mode would break. |
@seanyesmunt We lose the flexibility of using |
@NetOperatorWibby Ah I didn't know we couldn't use |
@seanyesmunt Do I need to do anything else to get this merged? |
I think the only thing left I've noticed is the text in the search bar is really hard to read on light mode. After that is fixed I think we can merge this. |
Done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm guessing there will be a few more things to fix before this is released but it's fine for now to begin testing.
Cool beans, I'll go through everything finely. |
This PR is unfinished, still have to go through the app and tweak.