-
Notifications
You must be signed in to change notification settings - Fork 0
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
Rewrite TwoLineTitleView to use autolayout #1
Rewrite TwoLineTitleView to use autolayout #1
Conversation
This reverts commit bbbb056 because the older solution results in a smaller binary size.
|
||
if subtitle?.isEmpty == false { | ||
maximumContentSizeCategory = .large | ||
containingStackView.addArrangedSubview(subtitleContainer) |
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.
Couldn't you just hide the subtitleContainer
instead of removing all the subviews and then adding them back?
titleLeadingImageView.isHidden = titleImage == nil | ||
|
||
setupTitleLine(titleContainer, label: titleLabel, trailingImageView: titleTrailingImageView, text: title, interactive: interactivePart.contains(.title), accessoryType: accessoryType) | ||
if titleLeadingImageView.image != nil { |
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.
Does titleLeadingImageView.image
change between lines 297 and 298, or could you just add it in setupTitleLine
? The trailing image is added there, and it would be better to just add them all in one place, in order, instead of add two of them, and later insert one into the front.
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.
titleLeadingImageView.image
doesn't change, but structuring it this way allows setupTitleLine
to also be responsible for the initial clearing out of the stack views. Also, adding leading image view infrastructure to setupTitleLine
results in a binary size increase of about 128 bytes.
At the end of the day, I don't think this should be a blocking change, so I'm gonna go with this as is.
This reverts commit 24ee01c. Despite the larger binary size, `activate([])` has better performance.
Platforms Impacted
Description of changes
This rewrite of
TwoLineTitleView
makes everything controlled by autolayout, which makes responding to things like screen rotations and content size changes much, much easier. No more manual refreshing! 🎉This also resolves the bug that made microsoft#1699 and microsoft#1731 incompatible, so we can bring this back as well.
There are several notable changes we made here:
EasyTapButton
.EasyTapButton
will register touches in a minimum area of 44x44, it'll never hear about it if it has a superview that's smaller in either dimension. We use d63c26b to ensure that minimum area.EasyTapButton
as a container view between theTwoLineTitleView
andcontainingStackView
, but for some strange reason the button wouldn't register taps. We got around this by overriding thetouches...
methods fromUIResponder
. There's no architectural overhead here sinceUIView
extendsUIResponder
.UIStackView
. As such, we no longer need the 20x20 chevrons, so we can get rid of them.Verification
Visual Verification
Pull request checklist
This PR has considered: