Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Automatically Switch to Dark Mode #410

Merged
merged 21 commits into from
Apr 15, 2021
Merged

Automatically Switch to Dark Mode #410

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
e5a8a93
Feat.: Add dark mode following device color scheme
Viyerelu23333 Mar 6, 2021
7f93bdc
Fix: Reduce flickers caused by loading courses
Viyerelu23333 Mar 6, 2021
d50be64
Feat.: Add method of change scheme to device color
Viyerelu23333 Mar 6, 2021
fa09a96
FIX: Change the implementation of dark mode switch
Viyerelu23333 Mar 7, 2021
8bc46a0
Fix: Change positions of toaster notifications
Viyerelu23333 Mar 8, 2021
33bb788
Feat.: Change color scheme without refreshing
Viyerelu23333 Mar 8, 2021
d3add46
Fix: Coding style enhancements
Viyerelu23333 Mar 9, 2021
50fcd63
Fix: Code readability
Viyerelu23333 Mar 12, 2021
1dda935
Feat.: Add dark mode following device color scheme
Viyerelu23333 Mar 6, 2021
571f7c9
Fix: Reduce flickers caused by loading courses
Viyerelu23333 Mar 6, 2021
ddc1c1d
Feat.: Add method of change scheme to device color
Viyerelu23333 Mar 6, 2021
fe6c7fe
FIX: Change the implementation of dark mode switch
Viyerelu23333 Mar 7, 2021
66cace1
Fix: Change positions of toaster notifications
Viyerelu23333 Mar 8, 2021
94c581a
Feat.: Change color scheme without refreshing
Viyerelu23333 Mar 8, 2021
7451fc3
Fix: Coding style enhancements
Viyerelu23333 Mar 9, 2021
dd76415
Fix: Code readability
Viyerelu23333 Mar 12, 2021
8950be6
Merge branch 'switch-dark-mode' of https://github.com/YACS-RCOS/yacs.…
Mar 23, 2021
66903a7
Fix: Resolved conflicts
Mar 23, 2021
dca88dd
Merge remote-tracking branch 'origin/master' into switch-dark-mode
Viyerelu23333 Mar 23, 2021
e1e95dd
fix: code duplicates and fix cookie reset function
Viyerelu23333 Apr 10, 2021
b2519d0
Merge branch 'master' into switch-dark-mode
Viyerelu23333 Apr 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 18 additions & 4 deletions src/web/src/App.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,17 +13,30 @@ export default {
name: "App",
components: {},
async created() {
const darkModeQueryBody = "(prefers-color-scheme: dark)";
if (this.$cookies.get("darkMode") === "true" ||
(this.$cookies.get("darkMode") === null &&
window.matchMedia(darkModeQueryBody).matches)) {
this.$store.commit(TOGGLE_DARK_MODE);
}

if (window.matchMedia &&
window.matchMedia(darkModeQueryBody).media !== 'not all') {
const queryColorScheme = window.matchMedia(darkModeQueryBody);
queryColorScheme.addEventListener('change', () => {
if (this.$cookies.get("darkMode") === null) {
this.$store.commit(TOGGLE_DARK_MODE);
}
});
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I understand, it looks like the first if block is checking the cookies to see if the user has an existing color-scheme preference and the second if block is setting the colorscheme to the device preference. right?

If so, lets maybe refactor a bit. Here's a few ideas so that we can get to:

async created() {
  this.syncDarkMode();
  ...
}
syncDarkMode() {
  if (this.isDarkModeCookieSet()) {
     this.syncDarkModeWithCookie();
  } else {
     this.syncDarkModeWithDevicePreferance();
  }
}

this way the code reads a bit simpler and we can seperate the concerns of checking if the dark mode is set, linking to cookie and matching device preference.

wyt?

Copy link
Contributor Author

@Viyerelu23333 Viyerelu23333 Mar 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your comments!
Yes, I think your refactoring ideas are great. Semantic function names are intuitive.

There are still two points:

  1. The first if checks the cookie or current scheme, and the second if checks the native support of color scheme: both if blocks will run, so it cannot be considered as a if-else block.
  2. Most browsers support window.matchMedia, so I deleted the testing for capabilities.

How about this: App.Vue

  async created() {
    this.darkModeInit();

    const querySemester = this.$route.query.semester;
    this.selectedSemester =
    // ......
  },
  methods: {
    darkModeInit() {
      const darkModeMediaQuery = window.matchMedia("(prefers-color-scheme: dark)");
      this.syncColorScheme(darkModeMediaQuery);
      this.registerColorSchemeListener(darkModeMediaQuery);
    },
    syncColorScheme(mq) {
      if (this.$cookies.get("darkMode")  === "true" ||
          (this.isFollowingDeviceColor() && mq.matches)) {
        this.$store.commit(TOGGLE_DARK_MODE);
      }
    },
    registerColorSchemeListener(mq) {
      mq.addEventListener("change", () => {
        if (this.isFollowingDeviceColor()) {
          this.$store.commit(TOGGLE_DARK_MODE);
        }
      });
    },
    isFollowingDeviceColor() {
      return this.$cookies.get("darkMode") === null;
    },
  },

Any ideas? Not commit yet.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reads much better! Lets take it one step further.

Describe a possible solution
If there is no darkMode cookie, query the prefers-color-scheme to check the current color scheme, and add the cookie according to the query result.

Lets only read the preferance once with .matches and not keep the listener open. This change would sit more inline with the UX that we are looking for from the issue definition

Codewise, we'd no longer need the eventListener as we'd only take the value once that you can wrap in a function getDarkModeDevicePreferance.

Then, I think we can follow the earlier logic that reads at the same abstraction level as the issue statement

sync created() {
  this.syncDarkMode();
  ...
}
syncDarkMode() {
  if (this.isDarkModeCookieSet()) {
     this.syncDarkModeWithCookie();
  } else {
     this.syncDarkModeWithDevicePreference(); // get pref, set dark mode, and save cookie
  }
}

Also small note on this predicates like this (not sure if itll be the same one):

(this.$cookies.get("darkMode")  === "true" || (this.isFollowingDeviceColor() && mq.matches)

but better to give it a function so that when reading you can stay at one level of abstraction (figuring out where darkmode preferances are coming from) and not worry about mq.matches and what that means and what a JS MediaQuery object is.

Lastly, strings "darkMode: and the "(prefers-color-scheme: dark)" can be extracted out like
https://github.com/YACS-RCOS/yacs.n/blob/master/src/web/src/controllers/Schedule.js#L7-L8
so that we catch string mismatches as compile time errors. You can throw these strings up to the top of the class as something like App.COOKIE_DARKMODE and App.MQ_COLORSCHEME or App.MEDIAQUERY_COLORSCHEME

wyt? or should we keep the responsiveness to colorscheme changes?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm way out of the loop here, but instead of only using prefers-color-scheme: dark when the darkMode cookie is null, could we just update the darkMode cookie state to match the prefers-color-scheme: dark state if it changes?

Copy link
Contributor Author

@Viyerelu23333 Viyerelu23333 Mar 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets only read the preferance once with .matches and not keep the listener open. This change would sit more inline with the UX that we are looking for from the issue definition

Codewise, we'd no longer need the eventListener as we'd only take the value once that you can wrap in a function getDarkModeDevicePreferance.

Then, I think we can follow the earlier logic that reads at the same abstraction level as the issue statement [...]

Sorry, I forgot to update the feature request. My bad. That was the original idea, and it only covers the change for new users who don't have cookies. On actual coding I found it is easy to implement a responsive color scheme as a third state... Personally I would like to preserve this feature.

Also small note on this predicates like this (not sure if itll be the same one):

(this.$cookies.get("darkMode")  === "true" || (this.isFollowingDeviceColor() && mq.matches)

but better to give it a function so that when reading you can stay at one level of abstraction (figuring out where darkmode preferances are coming from) and not worry about mq.matches and what that means and what a JS MediaQuery object is.

Agree. I thought this sentence shows a clear relationship of the conditions... But not optimal for reading it.
I will wrap it before the next push.

Lastly, strings "darkMode: and the "(prefers-color-scheme: dark)" can be extracted out like /src/web/src/controllers/Schedule.js: L7-L8 so that we catch string mismatches as compile time errors. You can throw these strings up to the top of the class as something like App.COOKIE_DARKMODE and App.MQ_COLORSCHEME or App.MEDIAQUERY_COLORSCHEME

Do you mean state.darkMode in store/index.js? Not sure if this will affect current usage of cookies...
Also, not sure if App.Vue has a class...

Copy link
Contributor Author

@Viyerelu23333 Viyerelu23333 Mar 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm way out of the loop here, but instead of only using prefers-color-scheme: dark when the darkMode cookie is null, could we just update the darkMode cookie state to match the prefers-color-scheme: dark state if it changes?

It is a bit confusing.
If I interpret your idea correct, darkMode cookie has a string for saving the user's current option for color scheme.

Also there is a boolean state.darkMode, which records the current site color scheme. This will change on each load, if the cookie was set to dark mode; or the user did not specify the scheme, it will change to follow the current device preference.

Copy link
Contributor

@jshom jshom Mar 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep you're right, extracting out into store/index.js should do it where you'd import COOKIE_DARKMODE.

Shouldn't change the behavior, only a code change

Proposed change goes from:

return this.$cookies.get("darkMode") === null;

to

return this.$cookies.get(COOKIE_DARKMODE) === null;

so easier to take advantage of autocomplete and reduce typos/silent errors

Copy link
Contributor Author

@Viyerelu23333 Viyerelu23333 Mar 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extracting out into store/index.js should do it where you'd import COOKIE_DARKMODE.
Shouldn't change the behavior, only a code change

Proposed change goes from:

return this.$cookies.get("darkMode") === null;

to

return this.$cookies.get(COOKIE_DARKMODE) === null;

so easier to take advantage of autocomplete and reduce typos/silent errors

Perhaps something like this code below?

store/index.js:

export const COOKIE_DARK_MODE = "darkMode";

Same import will apply to Header.Vue too.

App.Vue 
import { TOGGLE_DARK_MODE, SET_COURSE_LIST, COOKIE_DARK_MODE } from "@/store";
export const DARK_MODE_MEDIA_QUERY = "(prefers-color-scheme: dark)"; 
export default {
  methods: {
    syncColorScheme() {
      if (this.isCookieOrDeviceInDarkMode()) {
        this.$store.commit(TOGGLE_DARK_MODE);
      }
    },
    registerColorSchemeListener() {
      window.matchMedia(DARK_MODE_MEDIA_QUERY).addEventListener(
        "change", () => {
          if (this.isFollowingDeviceColor()) {
            this.$store.commit(TOGGLE_DARK_MODE);
        }
      });
    },
    isFollowingDeviceColor() {
      return this.$cookies.get(COOKIE_DARK_MODE) === null;
    },
    isCookieOrDeviceInDarkMode() {
      return this.$cookies.get(COOKIE_DARK_MODE) === "true" ||
              (this.isFollowingDeviceColor() && 
               window.matchMedia(DARK_MODE_MEDIA_QUERY).matches);
    }
  },
}


const querySemester = this.$route.query.semester;
this.selectedSemester =
querySemester && querySemester != "null"
? querySemester
: await getDefaultSemester();
const courses = await getCourses(this.selectedSemester);
this.$store.commit(SET_COURSE_LIST, courses);

if (this.$cookies.get("darkMode") == "true") {
this.$store.commit(TOGGLE_DARK_MODE);
}
},
computed: {
darkMode() {
Expand Down Expand Up @@ -63,6 +76,7 @@ export default {
{ property: "og:site_name", content: "YACS" },
{ property: "og:type", content: "website" },
{ name: "robots", content: "index" },
{ name: "color-scheme", content: "light dark" },
],
};
},
Expand Down
34 changes: 28 additions & 6 deletions src/web/src/components/Header.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,14 +36,11 @@
<b-nav-form id="darkmode-toggle-form" class="mr-md-2">
<b-form-checkbox
:checked="$store.state.darkMode"
:indeterminate="followDevice"
@change="toggle_style()"
switch
>
<div>
<!-- We need the outer div to keep the icon aligned with the checkbox -->
<font-awesome-icon icon="moon" />
</div>
</b-form-checkbox>
<font-awesome-icon @dblclick="toggle_default()" icon="moon" />
</b-nav-form>

<b-nav-item-dropdown right v-if="isLoggedIn">
Expand Down Expand Up @@ -97,7 +94,7 @@ import { mapGetters, mapState } from "vuex";
import SignUpComponent from "@/components/SignUp";
import LoginComponent from "@/components/Login";

import { TOGGLE_DARK_MODE } from "@/store";
import { TOGGLE_DARK_MODE, SAVE_DARK_MODE, RESET_DARK_MODE } from "@/store";

import { userTypes } from "../store/modules/user";

Expand All @@ -113,11 +110,36 @@ export default {
data() {
return {
semesterOptions: [],
followDevice: (this.$cookies.get('darkMode') === null),
};
},
methods: {
toggle_style() {
if (this.$cookies.get("darkMode") === null) {
this.$bvToast.toast(
`Double click moon icon to follow device's color scheme.`, {
title: 'Color Scheme Changed',
autoHideDelay: 2000,
noHoverPause: true,
variant: "info",
});
}
Viyerelu23333 marked this conversation as resolved.
Show resolved Hide resolved
this.$store.commit(TOGGLE_DARK_MODE);
this.$store.commit(SAVE_DARK_MODE);
this.followDevice = false;
},
toggle_default() {
if (this.$cookies.get("darkMode") !== null) {
this.$bvToast.toast(
`Toggled to follow device color.`, {
title: 'Color Scheme Changed',
autoHideDelay: 1000,
noHoverPause: true,
variant: "success",
});
}
this.$store.commit(RESET_DARK_MODE);
this.followDevice = true;
},
onLogIn() {
this.$refs["login-modal"].hide();
Expand Down
16 changes: 14 additions & 2 deletions src/web/src/store/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,9 @@ import { userModule, USER_NAMESPACE } from "./modules/user";
Vue.use(Vuex);

export const TOGGLE_DARK_MODE = "TOGGLE_DARK_MODE";
export const SET_COURSE_LIST = "SET_COURSE_LIST";
export const SAVE_DARK_MODE = "SAVE_DARK_MODE";
export const RESET_DARK_MODE = "RESET_DARK_MODE";
export const SET_COURSE_LIST = "SET_COURSE_LIST";

const store = new Vuex.Store({
state: {
Expand All @@ -15,7 +17,13 @@ const store = new Vuex.Store({
},
mutations: {
[TOGGLE_DARK_MODE](state) {
state.darkMode = !state.darkMode;
if (Vue.$cookies.get("darkMode" === null)) {
state.darkMode = window.matchMedia("(prefers-color-scheme: dark)").matches;
} else {
state.darkMode = !state.darkMode;
}
},
[SAVE_DARK_MODE](state) {
Vue.$cookies.set(
"darkMode",
state.darkMode,
Expand All @@ -26,6 +34,10 @@ const store = new Vuex.Store({
"Strict"
);
},
[RESET_DARK_MODE](state) {
Vue.$cookies.remove("darkMode");
state.darkMode = window.matchMedia("(prefers-color-scheme: dark)").matches;
},
[SET_COURSE_LIST](state, classes) {
state.courseList = classes;
},
Expand Down