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.

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

Feat/sync cookie #188

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

Hossein-Mirazimi
Copy link

this MR about sync localStorage with cookie

@netlify
Copy link

netlify bot commented Mar 17, 2023

Deploy Preview for nuxt-color-mode canceled.

Name Link
🔨 Latest commit 9ee19d9
🔍 Latest deploy log https://app.netlify.com/sites/nuxt-color-mode/deploys/64fc45595db70e00087dd5d2

@Atinux
Copy link
Contributor

Atinux commented Mar 19, 2023

Why adding the cookie sync @Hossein-Mirazimi ?

@Hossein-Mirazimi
Copy link
Author

Why adding the cookie sync @Hossein-Mirazimi ?

because in some cases may need to get preference value from cookies instead of local storage
#186

@tasiotas
Copy link

tasiotas commented Mar 19, 2023

Yep, for subdomain

When I visit example.com and set dark mode, it won't get carried over to subdomain.example.com without cookies

Copy link
Contributor

Atinux commented Mar 20, 2023

What about GPRD if we use cookies then? For EU websites they will have to display the cookies banner?

@tasiotas
Copy link

As far as I understand GDPR, it does not specify technology used for storage at all. It never uses term "cookie" or "local storage", it's about storing data for given reason.

So even with current method of using local storage, GDPR does apply.

The exceptions are outlined here, but bit hard to understand.

I would like to believe that color theme saved in cookie/storage is necessary to deliver what visitor has asked for. But is that accurate interpretation of EU law, I don't know.

@wokalek
Copy link

wokalek commented Jul 1, 2023

Really need this feature. But aware of GPRD must will be included when enable this setting.

@Hossein-Mirazimi
Copy link
Author

please help me add the GPRD rule for this

I don't know how to add this rule in the color-mode module

because this repo is just for managing the theme

If we should policy to set cookies for the theme

developers must do it in their project

Copy link
Contributor

Atinux commented Jul 25, 2023

It would be nice to have a GPRD expert to know what we should do about this

@hacknug
Copy link

hacknug commented Aug 4, 2023

According to GitHub it is not necessary: https://github.blog/2020-12-17-no-cookie-for-you/

EDIT: Also, both localStorage and sessionStorage are considered cookies from a legal stand point so I don't think this PR
introduces any new legal liabilities.

What we should probably discuss is whether or not this module should store any preferences before the user interacts with an element that changes $colorMode.preference.

I also think it might be better to add a mode option to the module that defaults to localStorage and allows 'localStorage'|'sessionStorage'|'cookies'. This way anyone can decide where to store these preferences and we don't need to touch multiple stores (not sure if this change would affect other modules that could depend on this though).

@Hossein-Mirazimi
Copy link
Author

please review my changes

function setPreferenceToStorage (storageType: typeof storage, preference: string) {
switch (storageType) {
case 'cookie':
window.document.cookie = storageKey + '=' + preference

Choose a reason for hiding this comment

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

Need ability to pass extra cookie options such as expires, max-age, path, samesite, secure, etc.

@Jordan-Ellis
Copy link

Since this pull request adds the ability to choose the storage type, it would be great if you could also specify a custom storage provider. For example, I'm using ionic and would prefer to store this in the native settings instead of local storage.

Something along the lines of this. The only thing I'm not sure of is where you would place the provider.

// Custom settings store. Could be pinia store, native settings library, etc.
const mySettings = useMySettings() 

// Settings provider passed to color mode
const colorModeSettingProvider = {
    getPreference() {
        return mySettings.get("theme")
    },
    setPreference(value) {
        mySettings.set("theme", value)
    }
}

Copy link
Contributor

Atinux commented Sep 18, 2023

It is a nice idea @Jordan-Ellis but the biggest issue is that it is written inside the inject script:

const preference = (window && window.localStorage && window.localStorage.getItem && window.localStorage.getItem('<%= options.storageKey %>')) || '<%= options.preference %>'

This code is added in the <head> of the project in order to properly handle static site generation to avoid having a color switch on hydration. This script can only be JavaScript code that run in the browser and don't have access to any of the Vue runtime.

@Jordan-Ellis
Copy link

Ah, I didn’t realize that! Should that be moved to a separate issue, or is that beyond the scope of the project right now?

@manniL
Copy link

manniL commented Nov 25, 2023

Ah, I didn’t realize that! Should that be moved to a separate issue, or is that beyond the scope of the project right now?

@Jordan-Ellis yup 👍


// @ts-ignore
function getStorageValue(storageType, storageKey) {
if (typeof window === 'undefined') return null;
Copy link

Choose a reason for hiding this comment

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

Suggested change
if (typeof window === 'undefined') return null;
if (window?.document === 'undefined') return null;

The other code will break on build.

@manniL
Copy link

manniL commented Nov 25, 2023

Another missing feature would be be adding cookie reading to plugin.server.ts to already have the info on the server (if a cookie is set).

Created a follow-up in #221 (have no permissions to edit this)

@manniL manniL mentioned this pull request Nov 25, 2023
@Nisthar
Copy link

Nisthar commented Mar 5, 2024

I got this error when using subdomains in my nuxt app. The color scheme was working fine on the main domain but not on the subdomains. Are there any workarounds for this issue or should i stop using nuxtui atm?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants