Replacing existing colors with npm module, and refactored Sass #2045

2018-10-17T00:33:03+02:00

NetOpWibby commented

2018-10-17 00:33:03 +02:00

(Migrated from github.com)

~~This PR is unfinished, still have to go through the app and tweak.~~

~_This PR is unfinished, still have to go through the app and tweak._~

❤️ 1

NetOpWibby (Migrated from github.com) reviewed 2018-10-18 23:07:16 +02:00

src/renderer/component/wunderbar/view.jsx

					
				@ -100,17 +100,15 @@ class WunderBar extends React.PureComponent<Props> {

				    if (suggestion) {

				      if (suggestion.type === 'search') {

				        onSearch(query, resultCount);

				      } else if (isURIValid(query)) {

NetOpWibby (Migrated from github.com) commented

2018-10-18 23:07:16 +02:00

The linter changed this file, not me.

NetOpWibby (Migrated from github.com) reviewed 2018-10-18 23:07:39 +02:00

src/renderer/redux/selectors/settings.js

NetOpWibby (Migrated from github.com) commented

2018-10-18 23:07:39 +02:00

Pretty sure this needs fixing.

neb-b commented

2018-10-19 19:17:24 +02:00

(Migrated from github.com)

This is looking great. I really like the new font. I found a few issues and also have a few more generic comments.

The input borders are gone. It looks like it can't find the variable
The modal opacity makes it hard to read text actual content on the modal
It's hard to read the help text next to the search suggestion
The exact url match on the search page needs to be updated for the new dark mode
Input/label input color needs to be updated for dark mode
The table row headings are hard to see on dark mode, also the grey on grey text is hard to read on even numbered rows

This is looking great. I really like the new font. I found a few issues and also have a few more generic comments. - The input borders are gone. It looks like it can't find the variable - The modal opacity makes it hard to read text actual content on the modal - It's hard to read the help text next to the search suggestion - The exact url match on the search page needs to be updated for the new dark mode - Input/label input color needs to be updated for dark mode - The table row headings are hard to see on dark mode, also the grey on grey text is hard to read on even numbered rows

neb-b commented

2018-10-19 19:31:44 +02:00

(Migrated from github.com)

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.

https://webaim.org/resources/contrastchecker/

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. https://webaim.org/resources/contrastchecker/

neb-b (Migrated from github.com) reviewed 2018-10-19 19:33:54 +02:00

src/renderer/redux/selectors/settings.js

neb-b (Migrated from github.com) commented

2018-10-19 19:33:54 +02:00

We can remove this now

NetOpWibby commented

2018-10-19 22:18:38 +02:00

(Migrated from github.com)

@seanyesmunt Pushed up some changes. The $lbry-teal-5 color (#2f9176) is being used for all buttons now, due to its excellent contrast. It doesn't pass WCAG AAA though, only WCAG AA.

#1f614d passes all tests but it's also pretty dark.

@seanyesmunt Pushed up some changes. The `$lbry-teal-5` color (`#2f9176`) is being used for all buttons now, due to its excellent contrast. It doesn't pass WCAG AAA though, only WCAG AA. `#1f614d` passes all tests but it's also pretty dark.

neb-b commented

2018-10-19 22:48:14 +02:00

(Migrated from github.com)

@NetOperatorWibby I don't think we have to pass all, teal-5 looks great. Way easier to read compared to teal-3

@NetOperatorWibby I don't think we have to pass all, `teal-5` looks great. Way easier to read compared to `teal-3`

👍 1

neb-b commented

2018-10-19 22:59:57 +02:00

(Migrated from github.com)

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)
And the markdown input on the publish page

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) And the markdown input on the publish page

NetOpWibby commented

2018-10-19 23:39:00 +02:00

(Migrated from github.com)

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.

😕 1

NetOpWibby commented

2018-10-20 00:17:07 +02:00

(Migrated from github.com)

This is another good site for contrast testing: https://colorable.jxnblk.com/ffffff/2f9176

❤️ 1

neb-b commented

2018-10-22 02:54:34 +02:00

(Migrated from github.com)

A few last issues (I think)

The startup screen/snackbar/text highlighting should use teal-5 too

A few last issues (I think) The startup screen/snackbar/text highlighting should use `teal-5` too

neb-b commented

2018-10-22 02:54:50 +02:00

(Migrated from github.com)

A few last issues (I think)

The startup screen/snackbar/text highlighting should use teal-5 too

A few last issues (I think) The startup screen/snackbar/text highlighting should use `teal-5` too

neb-b (Migrated from github.com) reviewed 2018-10-22 03:08:50 +02:00

src/renderer/scss/component/_button.scss

					
				@ -28,0 +108,4 @@

				  &.btn--header-publish {

				    background-color: $lbry-teal-5;

				  }

neb-b (Migrated from github.com) commented

2018-10-22 03:08:47 +02:00

Not super sure if we need this but I think that was for specificity

neb-b commented

2018-10-22 17:49:13 +02:00

(Migrated from github.com)

@NetOperatorWibby After looking at some of the code more closely I'm starting to think we should keep the vars file. That would avoid the need for using nested rules in the dark theme scss file.

Currently in some cases, if any layout changes, the dark mode would break.

@NetOperatorWibby After looking at some of the code more closely I'm starting to think we should keep the `vars` file. That would avoid the need for using nested rules in the dark theme scss file. Currently in some cases, if any layout changes, the dark mode would break.

NetOpWibby commented

2018-10-22 18:04:33 +02:00

(Migrated from github.com)

@seanyesmunt We lose the flexibility of using rgba functions and end up having more colors than necessary. I don't think layout changes are a strong argument. Dark mode would have to be checked out if changes are made regardless.

@seanyesmunt We lose the flexibility of using `rgba` functions and end up having more colors than necessary. I don't think layout changes are a strong argument. Dark mode would have to be checked out if changes are made regardless.

neb-b commented

2018-10-22 18:06:27 +02:00

(Migrated from github.com)

@NetOperatorWibby Ah I didn't know we couldn't use rgba functions with that. Ok I'm fine with it. 👍

@NetOperatorWibby Ah I didn't know we couldn't use `rgba` functions with that. Ok I'm fine with it. 👍

NetOpWibby commented

2018-10-22 18:14:18 +02:00

(Migrated from github.com)

@seanyesmunt Do I need to do anything else to get this merged?

neb-b commented

2018-10-22 18:15:30 +02:00

(Migrated from github.com)

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.

NetOpWibby commented

2018-10-22 18:22:44 +02:00

(Migrated from github.com)

Done

👍 1

neb-b (Migrated from github.com) approved these changes 2018-10-22 18:32:40 +02:00

neb-b (Migrated from github.com) left a comment

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.

NetOpWibby commented

2018-10-22 18:46:04 +02:00

(Migrated from github.com)

Cool beans, I'll go through everything finely.