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

Rewrite TwoLineTitleView to use autolayout #1

Merged
merged 38 commits into from
May 12, 2023

Conversation

amgleitman
Copy link
Owner

@amgleitman amgleitman commented May 10, 2023

Platforms Impacted

  • iOS
  • macOS

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:

  • We no longer use EasyTapButton.
    • Although 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.
    • We originally wanted to use a single EasyTapButton as a container view between the TwoLineTitleView and containingStackView, but for some strange reason the button wouldn't register taps. We got around this by overriding the touches... methods from UIResponder. There's no architectural overhead here since UIView extends UIResponder.
  • As per a closer look at the design spec, the title and subtitle now use 16x16 and 12x12 chevrons respectively. We previously leaned on the padding on the images, but this is now handled by the spacing inherent to UIStackView. As such, we no longer need the 20x20 chevrons, so we can get rid of them.
  • One noticeable visual change for center-aligned titles (as per another closer look at the design spec) is that we now treat each line as a single unit. For more information, see the third and fourth examples under "Made from UINavigationItem" in the verification screenshots. Making this option configurable can be added as an option later if there's an ask for it.

Verification

Visual Verification
Large content size 2XL content size
Simulator Screen Shot - iPhone 14 Pro - 2023-05-09 at 21 06 39 Simulator Screen Shot - iPhone 14 Pro - 2023-05-09 at 21 06 49

Pull request checklist

This PR has considered:

  • Light and Dark appearances
  • iOS supported versions (all major versions greater than or equal current target deployment version)
  • VoiceOver and Keyboard Accessibility
  • Internationalization and Right to Left layouts
  • Different resolutions (1x, 2x, 3x)
  • Size classes and window sizes (iPhone vs iPad, notched devices, multitasking, different window sizes, etc)
  • iPad Pointer interaction
  • SwiftUI consumption (validation or new demo scenarios needed)
  • Objective-C exposure (provide it only if needed)

@amgleitman amgleitman requested a review from huwilkes May 10, 2023 21:55
This reverts commit bbbb056 because
the older solution results in a smaller binary size.

if subtitle?.isEmpty == false {
maximumContentSizeCategory = .large
containingStackView.addArrangedSubview(subtitleContainer)

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 {

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.

Copy link
Owner Author

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.
@amgleitman amgleitman merged commit f460f62 into navbar-merge-main-2023-05-04-minus-1699 May 12, 2023
@amgleitman amgleitman deleted the new-2ltv branch May 12, 2023 02:43
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.

4 participants