[MBL-2913] New Floating Tab Bar #2670
Conversation
scottkicks
force-pushed
the
scott/mbl-2913/floating-tab-bar
branch
from
aae4218 to
9b094a0
Compare
scottkicks
requested review from
a team and
amy-at-kickstarter
and removed request for
a team
There was a problem hiding this comment.
A couple blocking changes:
- Use KDS spacing constants
- Removed the synchronized folder
- (If possible) don't change the view hierarchy of private views in UITabBar, keep the code just to layout and design tweaks
Also, can you throw a gif or screenshot in this review? Just for posterity.
| static let tabBarShadowOpacity: Float = 0.28 | ||
| static let tabBarShadowRadius: CGFloat = 28 | ||
| static let tabBarShadowOffsetY: CGFloat = 8 | ||
| static let tabBarVerticalPadding: CGFloat = 8 |
There was a problem hiding this comment.
These should use KDS spacing constants. Are these designs from the Figma? You should be able to map them 1:1 to Spacing.unit_0x
| } | ||
|
|
||
| required init?(coder: NSCoder) { | ||
| super.init(coder: coder) |
There was a problem hiding this comment.
Nit: This should be a fatalError since nobody should be using this initializer.
| super.init(coder: coder) | |
| fatalError("init(coder:) has not been implemented") |
| self.selectedTabBackgroundView.layer.cornerRadius = Constants.selectedTabBackgroundCornerRadius | ||
| self.selectedTabBackgroundView.clipsToBounds = true | ||
|
|
||
| addSubview(self.tabBarBackgroundView) |
There was a problem hiding this comment.
Nit: should call self.addSubview and self.sendSubviewToBack, just for style consistency. I'm surprised swiftformat didn't pick this up 🤔
| self.selectedTabBackgroundView.isHidden = isEmpty | ||
| self.tabsStackView.isHidden = isEmpty | ||
|
|
||
| guard isEmpty == false else { return } |
There was a problem hiding this comment.
Nit: instead of isEmpty and tabViews[0].center.y, you could call this like
| guard isEmpty == false else { return } | |
| guard let firstTab == self.tabViews.first else { return } | |
| let iconsCenterY = firstTab.center.y |
| dependencies = ( | ||
| A76E0A4C1D00C00500EC525A /* PBXTargetDependency */, | ||
| ); | ||
| fileSystemSynchronizedGroups = ( |
There was a problem hiding this comment.
Alas, we do not supported synchronized folders: see https://kickstarter.atlassian.net/browse/MBL-2676. This will break the build on CI.
|
|
||
| /// Make sure the tab StackView items are in the correct order. | ||
| /// UITabBar manages these views, so we re-sync them during layout. | ||
| private func syncStackViewArrangedSubviews(_ tabViews: [UIView]) { |
There was a problem hiding this comment.
Can you clarify what this is doing? If UITabBar is already managing these views, why are we adding/removing them from the view hierarchy? I'd expect us to just shuffle around their layout, instead.
My concern would be that mucking with the view hierarchy leads to weird bugs down the line.
There was a problem hiding this comment.
This is so that we can fully control the tab layout (floating UI, fixed width, equal spacing, etc.). UITabBar creates and manages the item views, but doesn’t give us a way to lay them out inside a custom container.
To avoid touching the view hierarchy entirely, I think we'd need to build a custom tab bar rather than subclass it.
| } | ||
| } | ||
|
|
||
| /// Returns the view for the given tab item`. |
There was a problem hiding this comment.
Nit: typo
| /// Returns the view for the given tab item`. | |
| /// Returns the view for the given tab item. |
📲 What
A new custom UITabBar subclass,
FloatingTabBarthat will replace our current tab bar UI.This just adds the new class. The next PR will implement it. It will be feature flagged.
SPIKE for reference
🤔 Why
We want a fancy new floating tab bar as shown here
🛠 How
UITabBarso we can override the UI but still make use of our existing tab navigation logic.UIStackViewto lay out the tabs, keeping the spacing logic as simple as possible.👀 See
See the POC gifs in the SPIKE
✅ Acceptance criteria
verified in my POC. this PR doesn't wire up this new class yet