close #186 (Automatic) dark mode #201

davidfloyd91 · 2019-06-03T17:07:49Z

Implements a fix to #186: when a user's OS is set to a dark theme, the popup's colors are shifted to match Firefox's dark theme.

Note that the fix is implemented using prefers-color-scheme, which reads from the OS, not the browser. So a user who's chosen a dark macOS theme, but a light Firefox theme, for example, will still see a popup that's formatted to match FF's dark theme.

Also note that if the OS theme is changed, the browser needs to be restarted for offline-qr-code to reflect that change.

rugk

Hi @davidfloyd91,
thanks for your first contribution to this project! 🎉 👍 🏅
I hope you'll like this project and enjoy hacking on it… 😃

Screenshot for reference:

So, are these colors somewhat official you've taken for this? I.e. from Firefox Photon guide?

rugk · 2019-06-03T17:26:32Z

CONTRIBUTORS.md

@@ -13,6 +13,7 @@
 - [@redagavin](https://github.com/redagavin)
 - [@Thammarith](https://github.com/Thammarith)
 - [@xlomonosvx](https://github.com/xlomonosvx)
+- David Floyd [@davidfloyd91](https://github.com/davidfloyd91)


Please sort it alphabetical, thanks.

rugk · 2019-06-03T18:09:29Z

src/popup/qrcode.css

+    background-color: var(--grey-30);
+  }
+  #qrcode-container {
+    background-color: var(--grey-60);


Oh another big problem. This change overrides the user's choice of the background color in the settings:

leads to:

So I am sorry, but it seems you need to adjust some JS-things, too…
(best solution would IMHO be that it only colors the background of the QR code dark, if you want it.)

So, hmm, I guess we need another change in one module even, where we get the default values from: TinyWebEx/AddonSettings#6

(see also the Readme, it provides an extensive doc on how it's used.)

That color is set in JS.

davidfloyd91 · 2019-06-03T18:31:57Z

Sorry about the alphabetizing, I'd read somewhere to add yourself to the bottom. Fixed that.

The colors are from Photon, yes. The popup background ("Grey 60", #4a4a4f) matches other FF dark theme popups. The QR code background is Photon's "Grey 30" (#d7d7db), which just seemed a bit less blinding than the default white.

The overwriting is definitely an issue. Just removing the background-color from .qrcode doesn't appear to work, since there's not enough contrast (see screenshot below). So I'll look at the module you mentioned and fix it in JS.

rugk · 2019-06-03T18:35:46Z

Okay, so. So marking this as blocked by TinyWebEx/AddonSettings#6

rugk · 2019-06-03T18:51:00Z

I'd read somewhere to add yourself to the bottom

You're right, my mistake. Fixed: TinyWebEx/common@90ca64d

Again regarding this color thing:

In case I have not expressed it correctly – now in a better way: IMHO it would be best if the add-on could automatically "select" dark default colors (for the QR code) for the user in case of prefers-color-scheme = dark.
Regarding contrast: Yep, that needs to be assured, by default it should be quite optimal (use the settings page, it checks for that 😉). Also remember that not all QR code scanners can read inverted QR codes: Warning for inverted QR codes (when background color is dark and foreground light) #20
So that choice of color needs to be a wise one, here… 😉

davidfloyd91 · 2019-06-03T20:14:42Z

Sounds good. I think I've just about wrapped my head around the AddonSettings module, it should be easy enough to add a couple of default settings. I'll keep you posted if I run into issues

rugk · 2019-06-03T20:21:07Z

Yeah, thanks. That sounds great. Feel free to ask any questions then… (best in TinyWebEx/AddonSettings#6, if they are only about that feature there)

davidfloyd91 · 2019-06-04T17:45:28Z

Cross-posting this comment, since it's relevant to this issue: TinyWebEx/AddonSettings#6 (comment)

rugk · 2019-06-04T20:05:31Z

BTW: Next time, try to avoid doing a pull request from the master branch, because you can run into problems when you have a "non-clean" master that does not follow this repo here (i.e. "upstream"). See this article for details.

And you can (automatically) let issues close when a PR is merged. (Just edit your PR description.)

davidfloyd91 · 2019-06-05T15:42:13Z

BTW: Next time, try to avoid doing a pull request from the master branch, because you can run into problems when you have a "non-clean" master that does not follow this repo here (i.e. "upstream"). See this article for details.

Sorry about that. What's the best way to proceed? I ~~was going to merge~~ went ahead and merged the latest commits into master so you can review them, but if that could cause problems I'm happy to close the PR and clean up my master (probably just by re-forking the repo? since we're only talking about 20-something lines of code...)

rugk · 2019-06-05T16:56:53Z

No problem, just keep it as it is now…

rugk · 2019-06-05T16:57:29Z

src/common/modules/data/DefaultSettings.js

+     if (window.matchMedia('(prefers-color-scheme: dark)').matches) {
+        return "#d7d7db"; // Photon Grey 30
+     } else {
+        return "ffffff";


Did you actually test it(?), because…

Suggested change

return "ffffff";

return "#ffffff";

Embarrassing. Looks like tests worked because white is the default, I tried it in a ternary with null and the result was the same. Fixing for readability

rugk · 2019-06-05T17:03:39Z

src/popup/modules/UserInterface.js

+
+            // checks for OS dark theme and sets sets colors in text field
+            if (window.matchMedia('(prefers-color-scheme: dark)').matches) {
+                qrCodeText.style.backgroundColor = "#4a4a4f"; // Photon Grey 60 -- FF dark theme popup color


Why did you move that from CSS to JS now?
IMHO a combination would be just fine…

Everything which you can define statically, put into CSS. (as it was before)
And everything which needs JS-handling, use JS…

Moving back to CSS

rugk · 2019-06-05T17:05:08Z

src/common/modules/data/DefaultSettings.js

+        return "ffffff";
+     }
+ };
+
 const defaultSettings = Object.freeze({
    debugMode: false,
    popupIconColored: false,
    qrCodeType: "svg",
    qrColor: "#0c0c0d",


Also generally, I guess you could maybe also adjust the QR code color in case of "dark mode", so that it matches to the background color? Or is this really not needed? (did you check the contrast/tested with QR code scanners etc.)

I tried out different combinations, always testing for contrast and QR scannability (if that's a word?). This combination seemed to reduce the white glare, stay in line with Photon and stay scannable better than others. But there might be a better solution

rugk · 2019-06-05T17:27:40Z

src/common/modules/data/DefaultSettings.js

@@ -11,12 +11,22 @@
 * @const
 * @type {Object}
 */
+
+// checks for OS dark theme and returns the appropriate background color
+ const setQrBackgroundColor = () => {


As per our guide:

Avoid anonymous functions, which have no name (i.e. not really assigned) unless they do really do simple things.

(I've edited the md file now to clarify that a little…)

Generally, don't use arrow functions here. Either inline it into the actual object (I would choose so) or make it a real "usual" function and JSDOC-document it.

Actually in this case you may even be able to simplify that whole thing to one line: https://developer.mozilla.org/docs/Web/JavaScript/Reference/Operators/Conditional_Operator

Sorry, read that in the guide but forgot. Setting to ternary (but pulling window.matchMedia('(prefers-color-scheme: dark)').matches out into a const so the line doesn't go on forever)

rugk · 2019-06-05T19:19:46Z

Ouch, no, merged the wrong PR…

davidfloyd91 · 2019-06-05T19:21:35Z

Oh I was wondering. I'll submit one with the changes you mentioned now

davidfloyd91 · 2019-06-05T19:24:14Z

Or wait? Let me know what you want to do, here's the commit davidfloyd91@1639cef

rugk · 2019-06-05T19:27:39Z

So I reverted master… GitHub does not seem to reopen this PR… so…

Your chance to open a new PR (with a new branch, possibly now 😉)

Sorry for the inconvenience…

davidfloyd91 added 4 commits June 2, 2019 23:00

Add automatic dark theme to CSS

7157190

Add to CONTRIBUTORS.md

028b807

Change dark mode .qrcode background

1fa9de5

Add comment to CSS: prefers-color-scheme reads from OS

3e2c790

rugk requested changes Jun 3, 2019

View reviewed changes

Alphabetize contributors

e19a003

rugk mentioned this pull request Jun 3, 2019

Allow function to programmatically return default values TinyWebEx/AddonSettings#6

Open

rugk requested changes Jun 3, 2019

View reviewed changes

rugk added the blocked This issue is blocked by something from the outside and cannot be done currently. label Jun 3, 2019

Specify function for qrBackgroundColor in defaultSettings

6b63a5f

Change dark mode default qr code background color

889fa74

davidfloyd91 changed the title ~~(Automatic) dark mode~~ close #186 (Automatic) dark mode Jun 5, 2019

Remove dangling else in applyingQrColor

d69f14c

rugk requested changes Jun 5, 2019

View reviewed changes

Add @cornu-ammonis to contributors

618e549

rugk merged commit 618e549 into rugk:master Jun 5, 2019

davidfloyd91 mentioned this pull request Jun 5, 2019

Take two: close #186 (Automatic) dark mode #204

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

close #186 (Automatic) dark mode #201

close #186 (Automatic) dark mode #201

davidfloyd91 commented Jun 3, 2019

rugk left a comment

rugk Jun 3, 2019

rugk Jun 3, 2019

davidfloyd91 commented Jun 3, 2019

rugk commented Jun 3, 2019

rugk commented Jun 3, 2019 •

edited

Loading

davidfloyd91 commented Jun 3, 2019

rugk commented Jun 3, 2019

davidfloyd91 commented Jun 4, 2019

rugk commented Jun 4, 2019 •

edited

Loading

davidfloyd91 commented Jun 5, 2019 •

edited

Loading

rugk commented Jun 5, 2019

rugk Jun 5, 2019

davidfloyd91 Jun 5, 2019

rugk Jun 5, 2019

davidfloyd91 Jun 5, 2019

rugk Jun 5, 2019

davidfloyd91 Jun 5, 2019

rugk Jun 5, 2019

rugk Jun 5, 2019

davidfloyd91 Jun 5, 2019

rugk commented Jun 5, 2019

davidfloyd91 commented Jun 5, 2019

davidfloyd91 commented Jun 5, 2019

rugk commented Jun 5, 2019

close #186 (Automatic) dark mode #201

close #186 (Automatic) dark mode #201

Conversation

davidfloyd91 commented Jun 3, 2019

rugk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidfloyd91 commented Jun 3, 2019

rugk commented Jun 3, 2019

rugk commented Jun 3, 2019 • edited Loading

davidfloyd91 commented Jun 3, 2019

rugk commented Jun 3, 2019

davidfloyd91 commented Jun 4, 2019

rugk commented Jun 4, 2019 • edited Loading

davidfloyd91 commented Jun 5, 2019 • edited Loading

rugk commented Jun 5, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rugk commented Jun 5, 2019

davidfloyd91 commented Jun 5, 2019

davidfloyd91 commented Jun 5, 2019

rugk commented Jun 5, 2019

rugk commented Jun 3, 2019 •

edited

Loading

rugk commented Jun 4, 2019 •

edited

Loading

davidfloyd91 commented Jun 5, 2019 •

edited

Loading