adding screenshots and ability to change based on OS theme #65

silvererudite · 2019-10-21T10:00:19Z

as per issue #59 @rugk would you please check if it works. I am having some wierd issues after I change theme in OS and reload. Please get back if you can figure out the problem.

rugk · 2019-10-21T13:03:23Z

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

Next time maybe try to install/use an editorconfig plugin or so for your editor in case it is not built-in. We use that in our repo, so it should notice and use that.

You can (automatically) let issues close when a PR is merged by adding some "magic" text to your PR body. It would be kind, if you could edit your PR to change this.

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.

I am having some wierd issues after I change theme in OS and reload

Could you describe what issue you actually have? ~~AFAIK you need to restart Firefox to properly apply the theme, possibly.~~

rugk

Nice screenshots BTW. 😃

Also, what maybe is an issue for your bugs is that you also need to register a listener for change events, i.e. when the preferred color changes at runtime. Fortunately, you can subscribe to these events. See: https://developer.mozilla.org/en-US/docs/Web/API/MediaQueryList#Methods

And better move this functionality into a separate module (JS file). Just as any other included module. Maybe name it ColorSchemeModeHelper or so.

rugk · 2019-10-21T13:04:13Z

src/options/options.css

@@ -3,3 +3,11 @@
 .float-image {
  margin: 10px;
 }
+
+@media (prefers-color-scheme: dark) {
+


An empty CSS ruleset? I guess this is not really useful. So let's remove it.

rugk · 2019-10-21T13:04:47Z

src/options/options.js

@@ -20,3 +20,12 @@ RandomTips.init(tips).then(() => {
    RandomTips.setContext("options");
    RandomTips.showRandomTipIfWanted();
 });
+
+let dark= window.matchMedia('@media (prefers-color-scheme: dark)');


Use editorconfig -> that = style is a non-standard here. A space is missing. 😄

rugk · 2019-10-21T13:06:13Z

src/options/options.js

+if(dark){
+    document.getElementById('searchBarDemo').src="./img/emojiSearchDog_dark.png";
+}
+else if(light){


Again, please adhere to the code style and use editorconfig (in one line).

rugk · 2019-10-28T12:31:47Z

@silvererudite If you need any pointers/help or have questions, feel free to ask.

rugk · 2019-11-07T11:58:32Z

@silvererudite Are you still interested in completing this? If not, that's no problem, just add a notice, so I know what the current status of this PR is.

silvererudite · 2019-11-07T13:57:47Z

hi @rugk sorry it's taking me this long. Can u pls help me debug why adding the eventlistener for detecting theme change at runtime is not working?
p.s I will push a clean commit at the end with your other suggested changes once I can make this work. thanks alot for being patient

* is is addListener, not addEventListener * only register for dark version, not retrigger for light version * the .matchMedia() query string was wrong (not include `@media`) * some code style fixes

rugk · 2019-11-08T11:22:05Z

Okay, pulled your change and fixed some things. Specifically:

is is addListener, not addEventListener
only register for dark version, not retrigger for light version
the .matchMedia() query string was wrong (not include @media)

While testing please also note:

in Linux/GNOME at least, you need to restart Firefox, so the change cannot be live-reloaded (tell me if you experience something else)
This is not bad, but we have implemented it, at least, already. As I guess this is a bug in Firefox, I've reported it here: https://bugzilla.mozilla.org/show_bug.cgi?id=1595037

rugk

So please:

fix the remaining issues (maybe use eslint for that, if you can)
i.e. e.g. add a jsdoc to the new function.
maybe please move it into a new module (in ./modules) there

silvererudite · 2019-11-08T11:34:42Z

@rugk how do I synchronize the remote fork with my local clone after your commits? To explain a bit, I want to make my local copy of the repo updated with your changes in the fork what is the Git command to do that? Or is there any other way to do so

…developer edition

silvererudite · 2019-11-08T14:33:03Z

Hi @rugk, was able to synchronize (turns out it's easier than I thought in vscode😀)I have pushed hopefully the final commit for the issue. Now this PR closes #59
As you suggested I used eslint for this commit now. I have also tested it using Firefox Dev edition 71.0 and it works fine. Please take a look and get back if I missed something.

rugk · 2019-11-08T17:46:21Z

The answer was "just pull", but apparently you figured that out by yourself.

rugk · 2019-11-08T17:47:43Z

src/options/modules/ColorSchemeModeHelper.js

@@ -0,0 +1,16 @@
+/* this function changes screenshot in the options menu as per user's OS theme.By defualt 


Please see on how we structure all other modules, i.e.:

make an init method

import the module as a dependency instead of adding a custom include line in the HTML

rugk · 2019-11-08T17:48:28Z

src/options/modules/ColorSchemeModeHelper.js

+/* this function changes screenshot in the options menu as per user's OS theme.By defualt 
+a user will see light themed screenshot but if the user has enabled dark theme
+in their OS the they would see dark-themed screenshot. It makes use of the following CSS media query
+and detects its change of state by attaching a js listener to it.


This comment is not JSDoc. Please document the function (too), it has a specific format. (just look it up, there are likely also plugins)

src/options/modules/ColorSchemeModeHelper.js

silvererudite · 2019-11-08T18:36:54Z

hi @rugk ...I fixed as per your latest review. not really sure about the module thing really. I tried to mimic the code in other module. Luckily browser testing passed. Please see if this is what you wanted.

rugk · 2019-11-08T18:46:21Z

src/options/modules/ColorSchemeModeHelper.js

@@ -0,0 +1,15 @@
+/**
+ * // does not work in Firefox currently due to https://bugzilla.mozilla.org/show_bug.cgi?id=1595037


no comment in comments 😜

Suggested change

* // does not work in Firefox currently due to https://bugzilla.mozilla.org/show_bug.cgi?id=1595037

* does not work in Firefox currently due to https://bugzilla.mozilla.org/show_bug.cgi?id=1595037

rugk · 2019-11-08T18:46:42Z

src/options/modules/ColorSchemeModeHelper.js

@@ -0,0 +1,15 @@
+/**
+ * // does not work in Firefox currently due to https://bugzilla.mozilla.org/show_bug.cgi?id=1595037


Also please explain the function's objective etc, as it is best practise for JSDoc.

rugk · 2019-11-08T18:46:59Z

src/options/modules/ColorSchemeModeHelper.js

@@ -0,0 +1,15 @@
+/**
+ * // does not work in Firefox currently due to https://bugzilla.mozilla.org/show_bug.cgi?id=1595037
+ * @param  {} darkQuery


type missing:

Suggested change

* @param {} darkQuery

* @param {object} darkQuery

(or maybe Object – pls check eslint, don't know it out of my head 😆 )

rugk · 2019-11-08T18:47:15Z

src/options/options.html

@@ -10,6 +10,8 @@

 		<script defer src="../common/common.js" type="module"></script>
 		<script defer src="./options.js" type="module"></script>
+		<script defer src="./modules/ColorSchemeModeHelper.js" type="module"></script>


can be removed now?

rugk · 2019-11-08T18:48:08Z

src/options/options.js

 // init modules
 CustomOptionTriggers.registerTrigger();
 AutomaticSettings.setDefaultOptionProvider(AddonSettings.getDefaultValue);
 AutomaticSettings.init();
+ColorSchemeModeHelper.changeScreenshotTheme();


nah… rather call it simply init here and structure the ColorSchemeModeHelper better to have a private function that does this internal stuff. Again, look at the other modules.

silvererudite · 2019-11-09T11:18:08Z

Hi @rugk pls check if this is what you wanted. All tests pass.

rugk · 2019-11-09T11:42:26Z

src/options/modules/ColorSchemeModeHelper.js

@@ -9,6 +14,7 @@ export function changeScreenshotTheme(darkQuery){
    } else {
        document.getElementById("searchBarDemo").src = "./img/emojiSearchDog_light.png";
    }
+    return ColorSchemeModeHelper.changeScreenshotTheme(darkQuery);


Did you actually test this change? (pls always do so)

Because AFAIK this should not work.

Please read https://hacks.mozilla.org/2015/08/es6-in-depth-modules/ or similar in order to understand how these modules work.

I tested it on Firefox dev edition by changing my OS theme and yes it worked fine. Are we talking about some other tests?

Well, no other tests, just usual dev testing.

Because it may work, but not without side-effects. Here is an exception thrown if you look into the debugger:

rugk · 2019-11-09T11:43:02Z

src/options/modules/ColorSchemeModeHelper.js

+    return ColorSchemeModeHelper.changeScreenshotTheme(darkQuery);
+}
+
+dark.addListener(changeScreenshotTheme);


As mentioned, put this into an init method.

I really have no idea how to do that. I tried to mimic how the existing modules were written. would you pls provide an example?

rugk · 2019-11-09T11:45:21Z

src/options/options.html

@@ -10,6 +10,8 @@

 		<script defer src="../common/common.js" type="module"></script>
 		<script defer src="./options.js" type="module"></script>
+


see the diff and remove these new empty lines, you've added pls

silvererudite · 2019-11-09T11:57:00Z

@rugk I tested these changes on FF dev edition and they worked as expected. However I'm not really getting what you mean when you say put the function in init method as I've not done sth like it in the past and not getting a clear idea form the existing modules. However I tried too mimic them as much as I could.
Would you pls provide an example for how to do so or maybe point me to some related reading so that i can learn?

rugk · 2019-11-09T12:00:15Z

Some tip:

the other options module has the init method basically here:

awesome-emoji-picker/src/options/modules/CustomOptionTriggers.js

Lines 341 to 349 in 83620e3

    
           /** 
        
            * Binds the triggers. 
        
            * 
        
            * This is basically the "init" method. 
        
            * 
        
            * @function 
        
            * @returns {Promise} 
        
            */ 
        
           export async function registerTrigger() {

I have to admit, that example is a little bad, because it is named differently 😆

here is e.g. a relatively simple module: https://github.com/rugk/awesome-emoji-picker/blob/master/src/popup/module/EmojiPicker.js

…ving some whitespaces

silvererudite · 2019-11-10T10:45:25Z

@rugk was I supposed to do sth like this?

rugk

Yeah…
That looks almost good to go, just one minor change.

rugk · 2019-11-10T13:26:29Z

src/options/modules/ColorSchemeModeHelper.js

+ * does not work in Firefox currently due to https://bugzilla.mozilla.org/show_bug.cgi?id=1595037
+ * @param  {Object} darkQuery
+ */
+const dark = window.matchMedia("(prefers-color-scheme: dark)");


Let's please move this line into the init method, so all code is in some function.

rugk · 2019-11-10T13:27:50Z

src/options/modules/ColorSchemeModeHelper.js

+    }
+}
+
+dark.addListener(changeScreenshotTheme);


Ah and this also into the ìnit function pls.

silvererudite · 2019-11-10T13:59:48Z

@rugk done

rugk · 2019-11-10T15:12:34Z

LGTM now, and you can BTW request a review at the top right on GitHub PRs. (for the next time 😃 )

I'll do some final tests and will merge it.

silvererudite · 2019-11-10T15:24:41Z

Thanks a lot for your patience @rugk . I'm glad I learnt a lot from bugging you :D
I look forward to contributing more to the project

rugk · 2019-11-10T15:40:10Z

So just did some small changes by myself now. However please take a look at them to learn what you've missed to do:

eslint requires @returns:

see the commit message for other details

* empty line * grammar in description * also space after brackets

rugk · 2019-11-10T15:44:02Z

Next time:
You can (automatically) let issues close when a PR is merged by adding some "magic" text to your PR body.

adding screenshots and ability to change based on OS theme

dff0a85

rugk requested changes Oct 21, 2019

View reviewed changes

attempt to detect changing theme

db405eb

rugk added 2 commits November 8, 2019 10:42

Fix some linting errors

db7a1ac

Fix some issues

028d9f6

* is is addListener, not addEventListener * only register for dark version, not retrigger for light version * the .matchMedia() query string was wrong (not include `@media`) * some code style fixes

rugk requested changes Nov 8, 2019

View reviewed changes

cleaned code, fixed other issues as per review and tested on Firefox …

6233ae7

…developer edition

rugk requested changes Nov 8, 2019

View reviewed changes

fixing jsdoc, and updating module based on project's code style

d6fb151

rugk requested changes Nov 8, 2019

View reviewed changes

more refactoring to stick to project's code style

8234c6a

rugk requested changes Nov 9, 2019

View reviewed changes

attempt to create an init method for changeScreenshotTheme() and remo…

0f65162

…ving some whitespaces

rugk requested changes Nov 10, 2019

View reviewed changes

rugk reviewed Nov 10, 2019

View reviewed changes

refactored init method

9f17b3f

rugk approved these changes Nov 10, 2019

View reviewed changes

Fix JSDoc returns and stylistic issues

7fea7e8

* empty line * grammar in description * also space after brackets

rugk merged commit 7fea7e8 into rugk:master Nov 10, 2019

rugk mentioned this pull request Nov 10, 2019

Show dark or light screenshot for address bar search #59

Closed

		@@ -0,0 +1,16 @@
		/* this function changes screenshot in the options menu as per user's OS theme.By defualt

		@@ -0,0 +1,15 @@
		/**
		* // does not work in Firefox currently due to https://bugzilla.mozilla.org/show_bug.cgi?id=1595037

	* // does not work in Firefox currently due to https://bugzilla.mozilla.org/show_bug.cgi?id=1595037
	* does not work in Firefox currently due to https://bugzilla.mozilla.org/show_bug.cgi?id=1595037

		@@ -10,6 +10,8 @@

		<script defer src="../common/common.js" type="module"></script>
		<script defer src="./options.js" type="module"></script>

adding screenshots and ability to change based on OS theme #65

adding screenshots and ability to change based on OS theme #65

Conversation

silvererudite commented Oct 21, 2019

rugk commented Oct 21, 2019 • edited Loading

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

Choose a reason for hiding this comment

rugk commented Oct 28, 2019

rugk commented Nov 7, 2019

silvererudite commented Nov 7, 2019

rugk commented Nov 8, 2019

rugk left a comment

Choose a reason for hiding this comment

silvererudite commented Nov 8, 2019

silvererudite commented Nov 8, 2019 • edited Loading

rugk commented Nov 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

silvererudite commented Nov 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rugk Nov 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

silvererudite commented Nov 9, 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

silvererudite commented Nov 9, 2019

rugk commented Nov 9, 2019

silvererudite commented Nov 10, 2019

rugk left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rugk Nov 10, 2019 • edited Loading

Choose a reason for hiding this comment

silvererudite commented Nov 10, 2019

rugk commented Nov 10, 2019

silvererudite commented Nov 10, 2019

rugk commented Nov 10, 2019

rugk commented Nov 10, 2019

rugk commented Oct 21, 2019 •

edited

Loading

silvererudite commented Nov 8, 2019 •

edited

Loading

rugk Nov 8, 2019 •

edited

Loading

rugk left a comment •

edited

Loading

rugk Nov 10, 2019 •

edited

Loading