fix: Add not-found states for all feature success scenes #17800
Conversation
neilkakkar
requested review from
liyiy and
benjackwhite
and removed request for
liyiy
| } | ||
|
|
||
| if (earlyAccessFeatureLoading) { | ||
| return <LemonSkeleton active /> |
There was a problem hiding this comment.
I think a is better than a single random skeleton... Skeletons should be used to build a mock UI in a loading state. If that's too much effort, spinner is the way to go I'd say.
There was a problem hiding this comment.
oh, interesting. For things like this one which is just querying postgres, and is relatively fast in the happy path, I find the spinner to be a bit more jarring, since it just flickers in the middle of the screen and goes away, whereas the skeleton is a bit more seamless.
How do you usually handle cases like these?
| earlyAccessFeature: { | ||
| loadEarlyAccessFeature: async () => { | ||
| if (props.id && props.id !== 'new') { | ||
| const response = await api.earlyAccessFeatures.get(props.id) | ||
| return response | ||
| try { | ||
| const response = await api.earlyAccessFeatures.get(props.id) | ||
| return response | ||
| } catch (error: any) { | ||
| actions.setEarlyAccessFeatureMissing() | ||
| throw error | ||
| } |
There was a problem hiding this comment.
This solution is totally fine for now. Only thing is, I think the better long term solution is to have the state object start in a null state. I think this is hard here as you use the same state for the kea form which would get in the way as it starts with the "new" state. Just an FYI for the future.
There was a problem hiding this comment.
Indeed, my previous commits tried to move to such a state, but it became a much bigger overhaul than I'd like.
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated2 snapshot changes in total. 0 added, 2 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
Problem
We weren't dealing with 404 states well. This should fix it
Changes
👉 Stay up-to-date with PostHog coding conventions for a smoother review.
How did you test this code?