Dark mode #214

sseficha · 2018-12-08T18:00:40Z

I've checked that this isn't a new swag opportunity proposal.
I've checked that this isn't a duplicate pull request.

Added a dark mode switch! Awaiting feedback :)

Fixes #106

aslafy-z

Thanks for your contribution! Here are some comments.
I have some more about the way you switch classes but have no time to explore ways now.
It needs some edits to be easily readable and perf efficient.

PS: I love your commit name

src/pug/includes/head.pug

src/styl/index.styl

src/js/index.js

vikaspotluri123

Left some comments, thanks for contributing! 🎉

Great work so far, just needs some polishing 💅

src/pug/includes/head.pug

src/styl/index.styl

src/js/index.js

vikaspotluri123 · 2018-12-08T18:17:37Z

src/js/index.js

+	for (var i =0; i<refs.length; i++) {
+		refs[i].classList.toggle("dark_refs");
+	}
+});


Could you run npm run lint and fix any issues? Thanks!

src/js/index.js

src/pug/includes/body.pug

src/js/index.js

src/styl/index.styl

vikaspotluri123 · 2018-12-08T18:25:48Z

One more thing in general - I don't like the small moon icon being a trigger, I think we should have something more substantial, with a label that tells you what's happening

sseficha · 2018-12-08T18:41:28Z

Thank you, I knew I was a long way from perfect, just needed some feedback to know I am on the right track. I will work on it soon. Also, I am sorry if I am not 100% technically correct with my commits, this is one of my first times contributing to a big project. :)

sseficha · 2018-12-25T19:15:49Z

Hope you like this version better. Cheers and merry christmas :)

sseficha · 2019-01-04T14:26:53Z

Waiting for some feedback. If something needs to change please tell me.

src/js/index.js

src/pug/includes/body.pug

aslafy-z · 2019-01-04T14:36:08Z

src/pug/includes/head.pug

@@ -7,14 +7,15 @@ meta(http-equiv="X-UA-Compatible" content="IE=edge")



These changes should be reverted.

src/styl/imports/dark.styl

src/styl/index.styl

src/js/index.js

OP cleared its preview work

aslafy-z

Just added a bunch of comments, do not hesitate to ask any question you need!

sseficha · 2019-01-07T14:26:18Z

I keep getting an error when running lint but I have not been able to see what's wrong whatsoever :/

src/js/index.js

aslafy-z · 2019-01-07T15:08:12Z

As I said in the comment, start by running npm run lint:fix.
Then if there is more errors than you can't fix manualy, post them as a comment so we can help you.

sseficha · 2019-01-07T15:10:08Z

> [email protected] lint:fix C:\Users\Solon\swag\swag-for-dev
> eclint fix '**/*' '!LICENSE' && xo --fix

events.js:183
      throw er; // Unhandled 'error' event
      ^

Error: File not found with singular glob: C:/Users/Solon/swag/swag-for-dev/'!LICENSE' (if this was purposeful, use `allowEmpty` option)
    at Glob.<anonymous> (C:\Users\Solon\swag\swag-for-dev\node_modules\glob-stream\readable.js:84:17)
    at Object.onceWrapper (events.js:315:30)
    at emitOne (events.js:116:13)
    at Glob.emit (events.js:211:7)
    at Glob._finish (C:\Users\Solon\swag\swag-for-dev\node_modules\glob\glob.js:197:8)
    at done (C:\Users\Solon\swag\swag-for-dev\node_modules\glob\glob.js:182:14)
    at Glob._processSimple2 (C:\Users\Solon\swag\swag-for-dev\node_modules\glob\glob.js:688:12)
    at C:\Users\Solon\swag\swag-for-dev\node_modules\glob\glob.js:676:10
    at Glob._stat2 (C:\Users\Solon\swag\swag-for-dev\node_modules\glob\glob.js:772:12)
    at lstatcb_ (C:\Users\Solon\swag\swag-for-dev\node_modules\glob\glob.js:764:12)
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! [email protected] lint:fix: `eclint fix '**/*' '!LICENSE' && xo --fix`
npm ERR! Exit status 1
npm ERR!
npm ERR! Failed at the [email protected] lint:fix script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     C:\Users\Solon\AppData\Roaming\npm-cache\_logs\2019-01-07T15_08_54_709Z-debug.log

I keep getting this error. Any idea why?

aslafy-z · 2019-01-07T15:32:24Z

Please rebase on master, I should have fixed your problem with #244

aslafy-z · 2019-01-07T15:34:13Z

moon.png and sun.png are still in the tree btw

sseficha · 2019-01-07T15:36:16Z

Should I stash the changes from #244 or include them too?

vikaspotluri123 · 2019-01-07T15:37:18Z

@aslafy-z I'll review after you approve 🙂

aslafy-z · 2019-01-07T15:42:48Z

Just merge your branch with upstream/master and fix conflicts

aslafy-z · 2019-01-07T16:55:24Z

I'm really sorry I messed up with your branch..
Can you do a git push --force of your current content? (WITHOUT PULLING before)
I got a backup here if needed: https://github.com/aslafy-z/swag-for-dev/tree/tmp/dark

GitHub doesn't let me reopen the PR till there is content in there so i'll after you pushed. I've could have fixed it myself if GitHub haven't close the PR letting me without repo access. Sorry for that :/

sseficha · 2019-01-07T19:13:56Z

I can only push to my upstream repo, with this PR being closed :/ . How should I go with this?

vikaspotluri123 · 2019-01-07T19:23:44Z

Since you're trying to merge from sseficha:master, you should force push your changes there

As a personal note, it's usually a good idea to create a branch rather than work directly off of master - here's a great guide from GitHub as to how a general workflow can be like: https://guides.github.com/introduction/flow/

sseficha · 2019-01-07T19:35:45Z

I am force pushing my changes to sseficha:master, but they aren't being synced here like the previous times. I was using different branches, but I was pushing my changes to master so that they will be reflected to the PR.

aslafy-z · 2019-01-09T12:00:18Z

src/styl/imports/dark-theme.styl

+	& h1, p, span
+		color white !important
+	& a
+		color #d11717 !important


This file misses a ending new line, you need to run lint !

=> Lint should be ran automatically on commit with husky and lint-staged modules
Why is it not working for you ?

I think you should do a clean merge or rebase from upstream because it looks that you misses files.
You may want to do a npm install again also.

I ran lint but it didn't show any errors! Guess it may be missing something. I will run npm again.

Does it works for you now?

src/pug/includes/body.pug

aslafy-z · 2019-01-09T12:06:42Z

src/js/index.js

@@ -104,3 +105,26 @@ window.addEventListener('load', () => {

 	cascade.call(window, true);
 });
+
+function setTheme(preference) {


There is lot of repetitions in this function and the following one, maybe we can do something to get some more generic theme switching engine ? staying simple

itaisteinherz · 2019-01-20T20:45:06Z

src/js/index.js

+	if (preference === 'dark') {
+		document.body.classList.add('dark');
+		colorSwitch.innerHTML = 'Light mode';
+	} else if (preference === 'light' || preference === null) {


Suggested change

} else if (preference === 'light' || preference === null) {

} else if (preference === 'light' || !preference) {

itaisteinherz · 2019-01-20T20:48:30Z

src/js/index.js

+function setTheme(preference) {
+	if (preference === 'dark') {
+		document.body.classList.add('dark');
+		colorSwitch.innerHTML = 'Light mode';


I think it would be better to set colorSwitch.textContent here since the value used is plaintext and doesn't contain any HTML (this also applies to line 115).

vikaspotluri123 · 2019-02-03T21:25:18Z

@sseficha Here's an example of ~ what I was thinking for dark mode: https://overreacted.io/

Good commit

27758dc

aslafy-z previously requested changes Dec 8, 2018

View reviewed changes

src/pug/includes/head.pug Outdated Show resolved Hide resolved

src/styl/index.styl Outdated Show resolved Hide resolved

src/js/index.js Outdated Show resolved Hide resolved

vikaspotluri123 suggested changes Dec 8, 2018

View reviewed changes

Merge branch 'master' of https://github.com/swapagarwal/swag-for-dev

b5b8865

plibither8 added the enhancement New feature or request label Dec 10, 2018

unknown added 7 commits December 17, 2018 18:01

EOF sync

47c8640

Solved merge conflict

2fd3f4d

Removed font-awesome

2784373

Dark mode 2.0

9a99ba9

Last Sync

31a43b5

Merge branch 'branch2.0'

dcd6859

Small syntax error

6a5c26e