fix: support minimal top bar for onboarding screens #18077
Conversation
|
@mariusandra you are the lucky recipient of my random review request :) |
There was a problem hiding this comment.
Looks good to me, except that snapshot... however it might be better to tag @Twixes , @thmsobrmlr or anyone else from the Frontend NoteForce team for UI 👀
There was a problem hiding this comment.
Somehow, this snapshot leaves me feeling empty.
There was a problem hiding this comment.
This looks good to me as well.. except for the snapshot :) Don't know what is going on with that.
Also I've left a comment inline for what I think would be more standard kea usage.
Regarding PostHog 3000: We're planning on releasing parts of the UI update soon and this will need some changes/updates as well. We (that is @PostHog/noteforce-3000) will take care of that and loop you in accordingly.
| minimalTopBar: [ | ||
| (s) => [s.fullscreen, s.sceneConfig], | ||
| () => { | ||
| const activeScene = sceneLogic.values.activeScene | ||
| const minimalTopBarScenes = [Scene.Products, Scene.Onboarding] | ||
| return activeScene && minimalTopBarScenes.includes(activeScene) | ||
| }, | ||
| ], |
There was a problem hiding this comment.
Using a connected selector instead of directly accessing sceneLogic.values.activeScene feels more "canonical kea" to me.
| minimalTopBar: [ | |
| (s) => [s.fullscreen, s.sceneConfig], | |
| () => { | |
| const activeScene = sceneLogic.values.activeScene | |
| const minimalTopBarScenes = [Scene.Products, Scene.Onboarding] | |
| return activeScene && minimalTopBarScenes.includes(activeScene) | |
| }, | |
| ], | |
| minimalTopBar: [ | |
| (s) => [s.activeScene], | |
| (activeScene) => { | |
| const minimalTopBarScenes = [Scene.Products, Scene.Onboarding] | |
| return activeScene && minimalTopBarScenes.includes(activeScene) | |
| }, | |
| ], |
This requires changing the connected values as well:
connect: {
values: [sceneLogic, ['sceneConfig', 'activeScene'], membersLogic, ['members', 'membersLoading']],
...
}
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 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. |
| @@ -20,7 +20,7 @@ function notifyFlagIfNeeded(flag: string, flagState: string | boolean | undefine | |||
|
|
|||
| function getPersistedFeatureFlags(appContext: AppContext | undefined = getAppContext()): FeatureFlagsSet { | |||
| const persistedFeatureFlags = appContext?.persisted_feature_flags || [] | |||
| return Object.fromEntries(persistedFeatureFlags.map((f) => [f, true])) | |||
| return Object.fromEntries(persistedFeatureFlags.map((f) => (Array.isArray(f) ? f : [f, true]))) | |||
There was a problem hiding this comment.
you can now default the persistedFeatureFlags to be an object (Record<string, string | boolean>) and get rid of the whole Object.fromEntries() schenanigans here and simplify things everywhere else too.
Just add true as value to previous default persisted flags
There was a problem hiding this comment.
nevermind, coming from env var as well, much more annoying to change 😅
|
This is so frustrating, why is the snapshot blank?? |
thmsobrmlr
force-pushed
the
fix/onboarding-top-nav
branch
from
bb4c94b to
db4769b
Compare
|
i can't figure out the issue with the stories rn but need to get this out so will make the story changes in another pr. |
Problem
The Products and Onboarding scenes used the std navigation, but this was distracting for people because most of the buttons don't work on these scenes.
Changes
Before:
After:
👉 Stay up-to-date with PostHog coding conventions for a smoother review.
How did you test this code?
I didn't see a good way to do a story for just the minimal nav since it's computed based on the scene that is showing. So i instead added a story for the Products scene, which does use this minimal nav bar, which would change if the top bar were inadvertently changed.
I also had to update useFeatureFlags so it could specify the flag value in the case of multivariate flags, so that works now too!