Only reload tabbar if something meaningful changed#2729
Conversation
ifosli
force-pushed
the
ifosli/stopConstantTabbarReload
branch
from
73d7082 to
1d968ff
Compare
ifosli
force-pushed
the
ifosli/stopConstantTabbarReload
branch
from
1d968ff to
8ea2fe8
Compare
|
|
||
| let floatingTabBarEnabled = featuredFlagChanged | ||
| .map { _ in featureFloatingTabBarEnabled() } | ||
| .skipRepeats() |
There was a problem hiding this comment.
This was the "one line change" that I thought this pr would be...
| .takeWhen( | ||
| Signal.merge( | ||
| self.userLocalePreferencesChangedProperty.signal, | ||
| featuredFlagChanged.ignoreValues() |
There was a problem hiding this comment.
Since the featureFlagChanged signal is considered in the standardViewControllers already, there's no reason it needs to be included here. And now that featureFlagChanged just means that feature flags might've changed, it cannot be included here.
| currentUser, floatingTabBarEnabled, .merge( | ||
| self.viewDidLoadProperty.signal, | ||
| self.userLocalePreferencesChangedProperty.signal, | ||
| featuredFlagChanged.ignoreValues() |
There was a problem hiding this comment.
Same here; removed featureFlagChanged since it's changed to mean "feature flags might've changed" instead of "feature flags that we care about did change"
| viewControllerNames.assertValueCount(1) | ||
|
|
||
| self.vm.inputs.currentUserUpdated() | ||
| withEnvironment(currentUser: User.template) { |
There was a problem hiding this comment.
Added the current user to the environment here to make currentUserUpdated mean something. Happy to delete it again if the goal of this test was to ensure we didn't reload view controllers if "currentUserUpdated()" got called but the user didn't change
| self.vm.inputs.userSessionEnded() | ||
|
|
||
| viewControllerNames.assertValueCount(4) | ||
| viewControllerNames.assertValueCount(3) |
There was a problem hiding this comment.
This count also changed because we're no longer double emitting on initial load
scottkicks
left a comment
scottkicks
left a comment
There was a problem hiding this comment.
Did some testing and it looks like there are some issues with these changes. See GIF
- The floating tab bar doesn't reload correctly. The icons are wonky.
- When switching back to the old tab bar there isn't a currently selected (highlighted) tab and the tab labels don't re-appear.
I'm not exactly sure what the fix is, but its definitely related to the changes made outside of adding .skipRepeats(). I checked that.
If you don't have the time or know why this is happening off the top of your head we can file a bug ticket separately since it isn't time sensitive right now.
Sidenote for me. I'm also seeing a new bug unrelated to your changes. Switching from old to new tab bar results in the search tab showing the Activities notification indicator. I created a separate ticket to fix that already. It isn't a release blocker since we're not rolling this out in this release.
Good catch! Looks like that weirdness only happens for logged out users, so good find! I've updated the way we use the |
2 participants
📲 What
Instead of reloading the tab bar every time the app's been backgrounded, ensure the view controllers only get regenerated if a) the tabbar flag changed value b) the login status changed or c) the user locale changed.
🤔 Why
Reloading the app each time it's been backgrounded is very annoying for users
👀 See
✅ Acceptance criteria