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 ↗︎
|
apps/docs/utils/storage.ts
Outdated
| * @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.
|
Swooping on this, great fix, needs to be done. Agree that the API is confusing so I'm going to cut the knot for now since it's internal. |
…lements-until-we-are-sure-we
steveruizok
temporarily deployed
to
deploy-staging
GitHub Actions
Inactive
steveruizok
added
the
docs-hotfix-please
steveruizok
deleted the
mitja/int-978-we-shouldnt-show-dismissable-elements-until-we-are-sure-we
branch
) 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? - [x] `improvement` --------- Co-authored-by: Steve Ruiz <steveruizok@gmail.com>
followup to #5570 wasn't working on the homepage / make the logic simpler anyway ### Change type - [ ] `bugfix` - [ ] `improvement` - [ ] `feature` - [ ] `api` - [x] `other`
followup to #5570 wasn't working on the homepage / make the logic simpler anyway ### Change type - [ ] `bugfix` - [ ] `improvement` - [ ] `feature` - [ ] `api` - [x] `other`
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