-
Notifications
You must be signed in to change notification settings - Fork 1k
Vertical mode floating button #5938
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
base: master
Are you sure you want to change the base?
Conversation
…ripe-ios into joyceqin/vertical-float-button
porter-stripe
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some general feedback:
- The animate in of the floating button seems a bit unusual in our sheet. Can we check with design if this is desired? Seems like it should just always be there and not slide in.
- Why not have the button always float vs sometimes sticky to the bottom? I feel it would be simpler and desired to make the button always float (at least in the PM list)
- Seems like we need some kind of shadow on the button now to give it some “height” over the content it sits on top of
| private lazy var bottomSpacer: UIView = { | ||
| let spacer = UIView() | ||
| spacer.translatesAutoresizingMaskIntoConstraints = false | ||
| let buttonHeight = primaryButton.bounds.height |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This captures the height of the primary button when bottomSpacer is first accessed, if the primaryButton hasn't been laid out yet the height will be 0 and the spacer will have the wrong height. Can you confirm the bottomSpacer is first accessed after the primaryButton has been laid out? If you're not sure, maybe try recalculating this dynamically or using a constraint approach.
| let spacer = UIView() | ||
| spacer.translatesAutoresizingMaskIntoConstraints = false | ||
| let buttonHeight = primaryButton.bounds.height | ||
| let totalBottomPadding = buttonHeight + buttonSpacing - 20 // account for stackView spacing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where does this -20 come from? If it's the stack view spacing, let's reference that value directly instead of hardcoding.
| stackViewBottomConstraint = stackView.bottomAnchor.constraint(equalTo: view.bottomAnchor) | ||
| stackViewBottomConstraint.priority = .defaultLow |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this constraint needed and why .defaultLow priority?
| stackView.addArrangedSubview(bottomSpacer) | ||
|
|
||
| UIView.animate(withDuration: 0.3) { | ||
| bottomSheet.view.layoutIfNeeded() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why in activate and deactivate floating button are we animating different views?
activateFloatingButton() animates the bottomSheet but in deactivateFloatingButton we animate self.view.
| override func viewWillDisappear(_ animated: Bool) { | ||
| deactivateFloatingButton() | ||
| super.viewWillDisappear(animated) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this needed? Also nit: but can we move the viewWillDisappear to the top-ish of the file if it's needed.
Summary
In vertical mode, if the primary button is completely off-screen, float the button at the bottom of the screen.
Motivation
https://jira.corp.stripe.com/browse/MOBILESDK-2909
Testing
Long list UI test asserting hittable, CI
Simulator.Screen.Recording.-.iPhone.12.mini.iOS16.4.-.2025-12-22.at.17.58.15.mov
Changelog
N/A