halfmoonui / halfmoon

Halfmoon is a highly customizable, drop-in Bootstrap replacement. It comes with three built-in core themes, with dark mode support for all themes and components.
https://www.gethalfmoon.com
MIT License
3.04k stars 118 forks source link

Use localStorage instead of cookies to store darkMode #30

Closed TheKingDave closed 4 years ago

TheKingDave commented 4 years ago

I would recommend using localStorage instead of cookies to store the darkMode state.

This stores the data only on the client and does not send it to the server all the time. It is easier to work with because it does not need additional parsing, or serialization. Additionally it is also well supported and meets the IE11 requirement.

Example with darkMode:

// Set darkMode
window.localStorage.setItem('darkModeOn', this.darkModeOn);
// Get darkMode
window.localStorage.getItem('darkModeOn');

I am very willing to create a PR.

halfmoonui commented 4 years ago

What would be the benefit of doing this? The way the cookie is stored at this moment does not require serialization or parsing.

More importantly, you actually want the darkModeOn to be readable on the server side. This is especially important if you are using backend frameworks like Laravel/Django/Rails where the view function generates and returns the template. On those types of stacks, the saved dark-mode preference can be set server side without JS (see: https://github.com/halfmoonui/halfmoon/issues/28#issuecomment-678469892)

TheKingDave commented 4 years ago

I understand the point made in #28.

But I could see future users of this framework who do not want their users to need to use cookies. Especially in times of GDPR. It could of course be argued that they are essential cookies.

Maybe the option to choose between cookie and localStorage could be a possibility.

With serialization/parsing I meant: https://github.com/halfmoonui/halfmoon/blob/37d569f79b73a95aa2b520414c869dba053ac22d/js/halfmoon.js#L47-L75

qgustavor commented 4 years ago

From a quick research GDPR don't applies only to cookies but localStorage too. For me the best option to avoid storing info about the user is not storing any information at all until the user interacts with the page, which I already suggested here. If the user interacts with the page wanting to change their dark mode preference then it's safe to assume that the user gave consent to store this information, which, from what I understand, is not a issue with GDPR.

Using a cookie and server side rendering it's possible to avoid the unwanted flash of light mode, but there are many solutions. One that avoids this issue without using real server side rendering is using service workers to emulate server side rendering (demo) which is, of course, too hard to implement. I think that the best option for the framework is providing a simple implementation that will solve this issue for most users and documenting what the optional JS library does so anyone can implement their own toggleDarkMode() function using localStorage, or Service Workers, or React, or Vue, or Choo... whatever.

halfmoonui commented 4 years ago

After thinking about this issue quite a bit, I think @qgustavor has the right idea. If GDPR applies to both cookies and localStorage, then there doesn't seem to be any advantage using localStorage. This could be made into a choice for the user, that is, pass in a parameter to select whether you want to save it in a cookie or in localStorage, but that seems like extra complexity without any real benefit.

I especially agree with the point that the framework should provide a simple implementation (ideally one that works for 95% of users in a way that they don't even have to think about it). The rest will have to override the method for more complex use cases. For example, I had to override the toggleDarkMode() method for the pages with iFrames in them on the documentation website.

Moreover, I would say the cookie definitely falls under essential. So, if I push what @qgustavor suggested in the next update (that is, cookie is not set until the user interacts with the page), there should be no issue going forward.

halfmoonui commented 4 years ago

The whole dark mode preference implementation has been overhauled in v1.1.0. Please refer to this page of the docs: https://www.gethalfmoon.com/docs/core-javascript-library/

Basically, what I wrote in my previous comment has been implemented in the framework. I will go ahead and close this issue. You can re-open it if you think the issue is still not resolved, or open another issue.