Add light theme #120

adtac · 2019-04-16T03:24:25Z

Here's what it looks like:

Closes #98

adtac · 2019-04-16T03:27:00Z

I'm not very good with frontend stuff, so please feel free to point out anything stupid lol

erayd

Initial impressions are good, but some of the features from the current styling are missing.

The biggest concern here is contrast. As you can see, the light theme lacks contrast between the matched portions of the text and the popup background, and the initial popup entry has the same background colour as the search field.

I would also like it if the active-item highlighting was a bit more subtle. At the moment the change feels much more distinct than it is in the dark theme. Perhaps darkening the entry background might help improve this, and resolve the background contrast issue above at the same time.

I think your choice of colours generally is excellent - other than the points above, contrast is largely excellent, and the colours look attractive and match well, without being too aggressive.

adtac · 2019-04-16T04:24:26Z

@erayd thanks for the review. Previously, match text colour was coupled with the hint background colour for some reason. I've decoupled the two now, so it should be much better. Here's what the popup looks like now:

I've amended the light theme commit as well.

adtac · 2019-04-16T04:33:17Z

(Made the badge a bit less dark to improve text contrast.)

erayd · 2019-04-16T04:48:20Z

Definitely improving - thanks for your work on this 👍 .

Can you add some colour (or better contrast) back to the domain badge in the search field? It's fading into the background a bit:

Other Stuff

There is no noticable difference in the search field background colour based on whether it's focused or not. Compare with the dark theme to see what I mean - the dark theme gets this bit right. If the search field were generally lighter, that might make this easier to resolve. I know I asked for some contrast between the search field and the entries, but you don't need quite as much as is there now, and if it's causing problems then toning it down a little might be a simple fix.

When moving focus from the search field to the first item (e.g. by pressing the down-arrow key), the first item gets much lighter, and gives the impression that it's just been deselected. Changing the colour or shade is fine, but it needs to indicate to the user that this element has gained focus, not lost it.

erayd · 2019-04-16T04:57:06Z

Here's another screenshot showing what it looks like with multiple custom stores configured:

adtac · 2019-04-16T05:11:16Z

@erayd updated. I was a bit hesitant to add non-grey colours to the mix as I didn't want it to be jarring, but I suppose it's alright to make the badge stand out a bit more.

Hover is now darker than active so that it's more intuitive. The search field gets subtly lighter when it goes out of focus as well.

erayd · 2019-04-16T05:15:48Z

Getting there. Now the domain badge has very low contrast with the 'remove-domain-badge' button though. Perhaps a light blue or something maybe?

adtac · 2019-04-16T05:20:42Z

Heh, you have a very keen eye, I missed that. Updated to a safer yellow :)

maximbaz

Very nice job! This is much better than I ever expected 👍

So far I only went through the code, but I would like to try it myself before merge, hopefully later tonight.

maximbaz · 2019-04-16T14:06:03Z

src/popup/colors.less

+    .part.login > .action:focus,
+    .part.login:focus > .name {
+        background-color: @hover-bg-color;
+        outline: none;


I think outline needs to be declared in popup.less, not here

maximbaz · 2019-04-16T14:11:47Z

src/popup/popup.js

+
+        document
+            .getElementsByTagName("html")[0]
+            .setAttribute("class", "colors-" + settings["theme"]);


Use .classList.add(`colors-${settings.theme}`), so that we don't accidentally override an existing class on <html> tag (now or in future).

In addition, please move this snippet to an earlier stage in the function, right after we retrieve settings. Right now I see a visible change of colors, when I open the popup, at first it is something white, and then in a split second it changes to dark theme.

Managed to catch a screenshot of it:

I confirm that this fixes the issue for my particular case, but to be on a safe side, can we perhaps even add class="colors-dark" directly in popup.html, to keep the theme by default dark even while we are loading settings? And this snippet would need to be adjusted to something like:

const root = document.getElementsByTagName("html")[0]; root.classList.remove("colors-light", "colors-dark"); root.classList.add("colors-${settings.theme}");

maximbaz · 2019-04-16T22:02:52Z

Finally got to try this! Here's some of my feedback:

I think we should be consistent in the way we highlight filter matches, if it is a different text color in dark theme, let's use text color in light theme too, not background color:
The "Copy" buttons look like they are disabled, perhaps because the icon color is too light on a light background. Do you guys have the same impression?
The "Copy" buttons and the "recency" badge (and maybe even the password store badge) look particularly hard to distinguish when the mouse is hovering the row:
I agree that the domain filter should probably be light blue instead of red or yellow, a couple of suggestions just to give an impression, what do you think?

background-color: #aad2f5;
color: #343a40;

background-color: #39a3ff;
color: white;

maximbaz · 2019-04-16T22:08:36Z

/cc a couple of people who I recall to be passionate about design and everyone who upvoted #98 so far, just to get a couple of extra opinions: @DamienCassou, @532910, @JanMa, @jakubtopic, @Tharre.

If anyone can't compile the branch to test, I'm willing to provide test builds, just say so.

I'd very much prefer to spend an extra time and get this right on the first try, so please, early feedback is very welcome.

Tharre · 2019-04-16T23:49:54Z

The buttons look grayed out/disabled indeed.

The darker blue looks the nicest to me, but you might want to tone down the red if you go with it.

erayd · 2019-04-17T04:57:19Z

I think we should be consistent in the way we highlight filter matches, if it is a different text color in dark theme, let's use text color in light theme too, not background color.

I disagree with this point - I actually think the way he's done the highlighting is spot on. Yes, it's different, but I don't see any issue with different themes being different, and the downside of a light pallette is it's harder to get good contrast with a text-colour change, the way the dark theme does it.

adtac · 2019-04-17T11:44:43Z

I think we should be consistent in the way we highlight filter matches, if it is a different text color in dark theme, let's use text color in light theme too, not background color:

It's currently both a different text colour and a different background colour. And as @erayd said, I personally think a small background contrast adds a lot to the visibility of the search matches. In fact, I wanted to update the dark theme to support this as well (I can still do this if others like the light theme's way of doing search matches better).

The "Copy" buttons look like they are disabled, perhaps because the icon color is too light on a light background. Do you guys have the same impression?

The "Copy" buttons and the "recency" badge (and maybe even the password store badge) look particularly hard to distinguish when the mouse is hovering the row

Agreed, I've updated this. It was a bit tricky because the classes use a SVG background with a hard-coded fill colour. I didn't want to create a separate SVG for the light theme as that's just an unnecessary maintenance overhead. As a result, the light theme is using CSS invert. While this is a perfectly valid solution for light/dark themes, implementing other themes will likely require hue rotate and other shenanigans.

I've also updated the badge to a dark blue. I agree with @maximbaz, @Tharre; it looks nicer:

I confirm that this fixes the issue for my particular case, but to be on a safe side, can we perhaps even add class="colors-dark" directly in popup.html, to keep the theme by default dark even while we are loading settings? And this snippet would need to be adjusted to something like

If the dark theme is the default, I agree. I've made the change you suggested, but I'd also like to start a discussion on what the default theme itself should be. I think there's a case to be made that the default theme should be light for two reasons:

Most browsers come with a default light theme, so it's a bit jarring to have an extension not follow the standard.
v2 used to have a light theme, so sticking to a light theme would make the upgrade easier from a muscle memory perspective.

ghost · 2019-04-17T17:17:05Z

So far I like the light theme a lot. Great work @adtac :-)
Maybe you could think about using the same yellow colours for highlighting as in the dark theme but this is just personal preference. I would also favour the light theme to be the default one, because light themes are also the default in Chrome and Firefox.

DamienCassou · 2019-04-17T17:52:55Z

I have used the light theme the whole day and I like it. Thank you for your work.

maximbaz · 2019-04-17T18:13:58Z

It's currently both a different text colour and a different background colour. And as @erayd said, I personally think a small background contrast adds a lot to the visibility of the search matches. In fact, I wanted to update the dark theme to support this as well (I can still do this if others like the light theme's way of doing search matches better).

If this is the only way to get a good contrast and everyone is happy, I'm okay with it too 🙂

I think the color alone works good in the dark theme, but I would be curious to try out your idea, tell me a pair of text and background colors that you are thinking for the dark theme 🙂

As a result, the light theme is using CSS invert. While this is a perfectly valid solution for light/dark themes, implementing other themes will likely require hue rotate and other shenanigans.

Nice solution!

I think there's a case to be made that the default theme should be light for two reasons:

Most browsers come with a default light theme, so it's a bit jarring to have an extension not follow the standard.

v2 used to have a light theme, so sticking to a light theme would make the upgrade easier from a muscle memory perspective.

I'm not sure it's entirely true, probably depends on the OS - my Firefox is dark and I don't remember changing any of its options (I only use it to test Browserpass, he-he), and my Chromium is also dark because it respects GTK theme. Plus many geeky people like dark themes, so among our users it's totally possible that the majority prefers dark. Finally, I don't think colors can influence the ease of upgrade in any way 😉

I think we'll stick to a default dark theme, as long as we have an option to switch to a light one, we are good 🙂

maximbaz · 2019-04-17T22:52:27Z

Getting much better!

I couldn't leave without trying to highlight with text color alone (#00bcff), IMHO looks quite alright, is it worse in your opinion than the highlight with background color?
If you still think the background highlight is much better, could it be made so that the blue background doesn't "touch" the store badge? In the picture below, ideally the blue background would stop maybe just a bit above the top of the "h" letter, but I feel there should be some space left between blue background and the store badge.
Would you agree that there's not enough contrast in this case, between blue background and the background of this entry? It doesn't look as bad on the enlarged screenshot, better try this in your browser:
When an entry is hovered, it feels the store badge also has small contrast (again doesn't look that bad on the enlarged screenshot, but try in your browser):
I'd recommend invert(85%) instead of 100%, makes the icons look not as bold:
The contrast between a focused search box and a pre-selected first line might also need additional improvement:
Is the "search icon" on purpose left without the "invert"? On the one hand it's not a button, on the other hand in the dark theme it has the same color as the buttons, and some might say it's almost invisible due to the low contrast with the background of the focused search input box:

adtac · 2019-05-02T23:17:29Z

No problem. Just ping me here when that is merged and you're good to go.

adtac · 2019-06-12T04:41:04Z

Hey @maximbaz, I haven't been following the project's development much. Any updates on the password generation feature that you wanted merged before this? I'd really like to merge this; looking at the issue page, I imagine a few others would like this too.

maximbaz · 2019-06-12T09:13:17Z

Hey, I apologize for the delay and the blocked state, I'll try to prioritize that work in the following days, I would also really like to get back to zero pending PRs in this project 🙂

adtac · 2019-08-12T00:49:25Z

@maximbaz ping :)

P.S. If you think the password generation feature will take more time than originally planned, I propose merging this patch first; just a suggestion :)

maximbaz · 2019-08-31T07:45:15Z

Apologies again. I agree, at this point let's just get this merged, #61 is taking way too much time than I planned. @erayd could you try again if you like the current colors, as well as do the code review? I'll do the same now.

maximbaz · 2019-08-31T09:21:29Z

For the background of the first entry highlighted by default I propose #d8dee3, a bit darker than the current #dee2e6, to add a bit of contrast to the blue highlight. Other than that, I'm ok with how it looks. @erayd?

erayd · 2019-08-31T10:58:54Z

@maximbaz I will go over this PR tomorrow.

DamienCassou · 2019-09-26T07:20:06Z

Hi guys. What is the status of this PR?

erayd · 2019-09-26T07:21:00Z

@DamienCassou I'm midway through reviewing it.

DamienCassou · 2019-09-26T07:23:15Z

Erayd <[email protected]> writes:

@DamienCassou I'm midway through reviewing it.

thank you very much!

…

-- Damien Cassou http://damiencassou.seasidehosting.st "Success is the ability to go from one failure to another without losing enthusiasm." --Winston Churchill

maximbaz

Let's get this finally merged! 🥇

maximbaz · 2019-10-19T16:59:45Z

Thanks @adtac for your work!

erayd · 2019-10-19T18:17:55Z

Yes, thank you @adtac, and sorry we took so long!

adtac · 2019-10-20T01:54:32Z

thank you, looks lovely!

adtac force-pushed the adtac/light branch 2 times, most recently from a7aeeae to 9c28b1f Compare April 16, 2019 03:25

erayd self-requested a review April 16, 2019 03:32

erayd requested changes Apr 16, 2019

View reviewed changes

adtac force-pushed the adtac/light branch from 9c28b1f to 1eaf41c Compare April 16, 2019 04:21

adtac force-pushed the adtac/light branch 3 times, most recently from 9da0e5f to 1b6680a Compare April 16, 2019 04:32

adtac force-pushed the adtac/light branch from 1b6680a to c8cdd82 Compare April 16, 2019 05:06

adtac force-pushed the adtac/light branch from c8cdd82 to 5cb3b49 Compare April 16, 2019 05:20

maximbaz requested changes Apr 16, 2019

View reviewed changes

maximbaz mentioned this pull request Apr 16, 2019

Add light theme #98

Closed

adtac force-pushed the adtac/light branch from 5cb3b49 to 2121730 Compare April 17, 2019 11:44

erayd added the blocked Cannot progress without first resolving something else this depends on label Aug 31, 2019

erayd assigned maximbaz Aug 31, 2019

erayd added this to the 3.2 milestone Aug 31, 2019

maximbaz removed the blocked Cannot progress without first resolving something else this depends on label Aug 31, 2019

erayd self-assigned this Aug 31, 2019

maximbaz modified the milestones: 3.2, 3.3 Sep 25, 2019

erayd approved these changes Oct 19, 2019

View reviewed changes

erayd added 3 commits October 20, 2019 04:42

Bump version in comment

0a4ac48

Include all less files, not just one

74b9944

Tweak colour

5e2cf2a

erayd approved these changes Oct 19, 2019

View reviewed changes

Include all less files for options too

f1c249e

maximbaz approved these changes Oct 19, 2019

View reviewed changes

maximbaz changed the title ~~Add themes support, add light theme~~ Add light theme Oct 19, 2019

maximbaz merged commit 592f123 into browserpass:master Oct 19, 2019

fkneist pushed a commit to fkneist/browserpass-extension that referenced this pull request Feb 12, 2022

Add light theme (browserpass#120)

aea197b

Add light theme #120

Add light theme #120

Conversation

adtac commented Apr 16, 2019

adtac commented Apr 16, 2019

erayd left a comment

Choose a reason for hiding this comment

adtac commented Apr 16, 2019 • edited Loading

adtac commented Apr 16, 2019

erayd commented Apr 16, 2019 • edited Loading

Other Stuff

erayd commented Apr 16, 2019

adtac commented Apr 16, 2019

erayd commented Apr 16, 2019 • edited Loading

adtac commented Apr 16, 2019

maximbaz left a comment

Choose a reason for hiding this comment

maximbaz Apr 16, 2019

Choose a reason for hiding this comment

maximbaz Apr 16, 2019

Choose a reason for hiding this comment

maximbaz Apr 16, 2019

Choose a reason for hiding this comment

maximbaz commented Apr 16, 2019

maximbaz commented Apr 16, 2019

Tharre commented Apr 16, 2019

erayd commented Apr 17, 2019

adtac commented Apr 17, 2019 • edited Loading

ghost commented Apr 17, 2019

DamienCassou commented Apr 17, 2019

maximbaz commented Apr 17, 2019

maximbaz commented Apr 17, 2019 • edited Loading

adtac commented May 2, 2019

adtac commented Jun 12, 2019

maximbaz commented Jun 12, 2019

adtac commented Aug 12, 2019

maximbaz commented Aug 31, 2019

maximbaz commented Aug 31, 2019

erayd commented Aug 31, 2019

DamienCassou commented Sep 26, 2019

erayd commented Sep 26, 2019

DamienCassou commented Sep 26, 2019 via email

maximbaz left a comment

Choose a reason for hiding this comment

maximbaz commented Oct 19, 2019

erayd commented Oct 19, 2019

adtac commented Oct 20, 2019 • edited Loading

adtac commented Apr 16, 2019 •

edited

Loading

erayd commented Apr 16, 2019 •

edited

Loading

erayd commented Apr 16, 2019 •

edited

Loading

adtac commented Apr 17, 2019 •

edited

Loading

maximbaz commented Apr 17, 2019 •

edited

Loading

adtac commented Oct 20, 2019 •

edited

Loading