Don't show dismissible elements until we are sure of their status #5570
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
MitjaBezensek
temporarily deployed
to
bemo-canary
GitHub Actions
Inactive
MitjaBezensek
temporarily deployed
to
deploy-staging
GitHub Actions
Inactive
| @@ -63,7 +63,9 @@ function CookieConsent({ | |||
| } | |||
| } | |||
|
|
|||
| if (hasConsent !== 'unknown') return null | |||
| if (hasConsent === 'not-initialized' || hasConsent !== 'unknown') { | |||
There was a problem hiding this comment.
does this extra logic of not-initialized get us anything here?
'not-initialized' !== 'unknown' so you can just leave this line as:
if (hasConsent !== 'unknown') return null
but then, why do we add not-initialized? :) seems like we might not need the extra complexity here
There was a problem hiding this comment.
True, we can drop the additional check.
We do need it tough. Go the https://tldraw.dev/, accept the cookies, then to a hard reload. You'll see the consent for a split second.
| * @public | ||
| */ | ||
| export function useLocalStorageState<T = any>(key: string, defaultValue: T, initialValue?: T) { | ||
| const [state, setState] = useState(initialValue ?? defaultValue) |
There was a problem hiding this comment.
i think something about overloading the initialized/not-initialized state makes this code hard to reason about. The issue in my head is that it starts with the initialValue but then the developer is supposed to notice that the defaultValue overrides it shortly thereafter in the useLayoutEffect.
i think it feels a little too complex/leaky to the caller for what this hook is doing.
i propose maybe making a separate state: [isInitialized, isNotInitialized]
and then this hook would return:
const [surveyStatus, setSurveyStatus, isInitialized] = useLocalStorageState<SurveyStatus>(
'devrel-survey',
'not-dismissed',
)
or alternatively:
const {surveyStatus, setSurveyStatus, isInitialized}
I think that would help us avoid any of the not-initialized values maybe accidentally getting stored or something.
There was a problem hiding this comment.
Sure, can take a look on how to improve the api of this. It feels like we definitely need to know if we read the value from the storage or not yet. We can't default to just showing the element, since that causes the issue.
Will probably take this up when I'm back from vacation.
There was a problem hiding this comment.
Agreed. Yeah, I'm just pushing on splitting it out from the enum of possible values so that the state of being loaded/not-loaded isn't tied to what is actually expected in the cookie value.
We used to default to "user didn't dismiss" this yet. Which meant that we always showed a dismissible element first, then we read the actual value, and we then might have dismissed the elements (if that's what the stored state dictated). We now have a "not initialized" state during which we don't show the elements.
Side effect of this change is that some users that have already dismissed these elements might see them again. I guess we could add some sort of migration of the state, but meh, doesn't seem worth the hassle?
Change type
improvement