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

Fix fixed footer experiment #4969

Merged
merged 4 commits into from
Aug 22, 2024
Merged

Fix fixed footer experiment #4969

merged 4 commits into from
Aug 22, 2024

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Aug 21, 2024

Haven't tested this very deeply but I think this should fix the fixed_bottom_bar experiment.

It takes an alternate approach to what I had before. The idea is to split the mode variable into headerMode and footerMode. Initially we always update them in unison (but read from headerMode unless we're specifically interested in the footer). Then we rip out the current conditional logic during read. Then we add some conditional logic during write, and only for the MainScrollProvider case which governs on-scroll showing/hiding. Other cases still set both.

Individual commits tell the story.

I think this should be enough.

Test Plan

Verify the old mode (without gate) works like in prod.

Verify the experiment doesn't break starter pack wizard and other screens where we intentionally hide the bottom bar.

For now, we'll always write them in sync. When we read them, we'll use headerMode as source of truth. This will let us keep footerMode independent in a future commit.
This isn't the right time to determine special behavior. Instead we'll adjust footerMode itself conditionally on the gate.
This lets us fork the implementation later just for this case.
This is the final piece. Normal calls to setMode() keep setting both header and footer, but MainScrollProvider adjusts the footer conditionally.
Copy link

render bot commented Aug 21, 2024

Copy link

Old size New size Diff
7.1 MB 7.11 MB 1.33 KB (0.02%)

@haileyok
Copy link
Contributor

Will want to see how it performs on-device (both platforms) once merged, but works good in simulator and code lgtm.

@gaearon gaearon merged commit b8dbb71 into main Aug 22, 2024
6 checks passed
@gaearon gaearon deleted the tweak-minmode branch August 22, 2024 22:27
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.

2 participants