♿️ Disable Dynamic Type on buttons #806

dusi · 2019-08-19T21:30:16Z

📲 What

Disables Dynamic Type support globally on our buttons.

🤔 Why

Due to quite a few bugs and also the fact that all our buttons resemble Apple Pay button very closely we're temporarily disabling Dynamic Type support.

The majority of our buttons did not use multiline labels and therefore supporting Dynamic Type would result in truncations anyways.

🛠 How

Simply by using UIKit's boldSystemFont API.

👀 See

Project page

Before 🐛	After 🦋

Before 🐛	After 🦋

Rewards

Before 🐛	After 🦋

Pledge screen

Before 🐛	After 🦋

To illustrate this slightly better here's how before and after looks with a bit of blur (that potentially highlights the distinction of each major element on the screen.

Before 🐛	After 🦋

👆 Notes

Please note that you might need to temporarily do the following changes to ProjectPamphlet+Helpers.swift due to an existing problem with our experiments..

public func userCanSeeNativeCheckout() -> Bool {
  return /* experimentNativeCheckoutIsEnabled() && */ featureNativeCheckoutIsEnabled()
}

✅ Acceptance criteria

CTA button on Project page does not support Dynamic Type
CTA button on Rewards cell does not support Dynamic Type
CTA button on Pledge screen does not support Dynamic Type
Every other button in the app (random sweep should be done) does not support Dynamic Type

dusi · 2019-08-19T22:24:02Z

Kickstarter-iOS/Views/Controllers/PledgeContinueViewController.swift

@@ -5,7 +5,7 @@ import Prelude
 final class PledgeContinueViewController: UIViewController {
  // MARK: - Properties

-  private let continueButton = MultiLineButton(type: .custom)
+  private let continueButton = UIButton(type: .custom)


We no longer need to use MultiLineButton here...in fact since we've previously added trimming we haven't been using it for a while (so this is just a cleanup).

I see we're only using this in one place, not time to remove MultiLineButton yet?

dusi · 2019-08-19T22:24:16Z

Kickstarter-iOS/Views/Controllers/PledgeContinueViewController.swift

-      |> UIButton.lens.title(for: .normal) %~ { _ in
-        Strings.Continue()
-      }
+      |> continueButtonStyle


Moving to // MARK: - Styles for consistency

I was thinking that we can start renaming the Styles pragma mark above bindStyles() to Styles Binding or Bind Styles, so it differs from the Styles on the bottom of the file. what do you think?

dusi · 2019-08-19T22:24:34Z

Kickstarter-iOS/Views/Controllers/PledgeContinueViewController.swift

+      Strings.Continue()
+    }
+    |> (UIButton.lens.titleLabel .. UILabel.lens.font) .~ UIFont.boldSystemFont(ofSize: 16)
+    |> (UIButton.lens.titleLabel .. UILabel.lens.lineBreakMode) .~ NSLineBreakMode.byTruncatingMiddle


Using consistent truncation in the middle for all our CTA buttons now

dusi · 2019-08-19T22:25:00Z

Kickstarter-iOS/Views/PledgeCTAContainerView.swift

-        isAccessibilityCategory,
-        amountAndRewardTitleStackViewIsHidden: self.titleAndSubtitleStackView.isHidden
-      )
+      |> pledgeCTAButtonStyle


We no longer need to know whether it's accessibility category or what state we're in

dusi · 2019-08-19T22:25:28Z

Kickstarter-iOS/Views/RewardCardContainerView.swift

@@ -104,8 +104,9 @@ public final class RewardCardContainerView: UIView {
    self.viewModel.outputs.pledgeButtonStyleType
      .observeForUI()
      .observeValues { [weak self] styleType in
-        guard let self = self else { return }
-        _ = self.pledgeButton |> styleType.style
+        _ = self?.pledgeButton


Using optional chaining for consistency between files

justinswart · 2019-08-19T23:02:30Z

Kickstarter-iOS/Views/PledgeCTAContainerView.swift

-      |> (UIButton.lens.titleLabel .. UILabel.lens.textAlignment) .~ NSTextAlignment.center
-      |> (UIButton.lens.titleLabel .. UILabel.lens.lineBreakMode) .~ lineBreakMode
-  }
+private let pledgeCTAButtonStyle: ButtonStyle = { button in


It shouldn't be necessary to set roundedStyle and borderWidth because these are included in our base button styles now. If we want all of our buttons to behave this way shouldn't we move these to the base style?

justinswart · 2019-08-20T00:12:17Z

Kickstarter-iOS/Views/Controllers/PledgeContinueViewController.swift

-    }
-    |> (UIButton.lens.titleLabel .. UILabel.lens.font) .~ UIFont.boldSystemFont(ofSize: 16)
-    |> (UIButton.lens.titleLabel .. UILabel.lens.lineBreakMode) .~ NSLineBreakMode.byTruncatingMiddle
+    |> ButtonStyleType.green.style


Not necessary to use the ButtonStyleType enum here, that is really just for our VMs to output a button style as something that we can do equality comparison on in tests. Swift doesn't support equality comparison on function types. So this can just be greenButtonStyle 👍

Scollaco · 2019-08-20T15:00:42Z

I've noticed, though, that the buttons on the credit card cells still support dynamic type. Was this intentional or supposed to be part of other PR?

Scollaco · 2019-08-20T15:07:41Z

Kickstarter-iOS/Views/Controllers/PledgeContinueViewController.swift

-      |> UIButton.lens.title(for: .normal) %~ { _ in
-        Strings.Continue()
-      }
+      |> continueButtonStyle


I was thinking that we can start renaming the Styles pragma mark above bindStyles() to Styles Binding or Bind Styles, so it differs from the Styles on the bottom of the file. what do you think?

Scollaco · 2019-08-20T15:26:31Z

Kickstarter-iOS/Views/PledgeCTAContainerView.swift

    let spacing: CGFloat = (isAccessibilityCategory ? Styles.grid(1) : 0)

    return stackView
-      |> \.axis .~ axis
+      |> \.axis .~ NSLayoutConstraint.Axis.horizontal


Was the compiler giving you trouble here? 😄 I think that |> \.axis .~ .horizontal is more readable and more used on the codebase. Also, we have more styling going on inside the bindStyles() function. To keep consistency and avoid doing the same thing in multiple places, I would either move this code to the bindStyles function (not recommended) or do a small refactor bringing the other styles to the bottom. (recommended, since that's how we are approaching styling).

It's not that the compiler would give us trouble but we've been seeing a lot of warnings lately related to Xcode's type checker...therefore at some point we've decided that for styles it would make more sense to be more explicit.

I'll make the styling more consistent by extracting bindStyles here.

Scollaco · 2019-08-20T15:32:17Z

Library/Styles/ButtonStyles.swift

-    |> UIButton.lens.titleLabel.font .~ UIFont.ksr_callout().bolded
-    |> (UIButton.lens.titleLabel .. UILabel.lens.textAlignment) .~ NSTextAlignment.center
-    |> UIButton.lens.contentEdgeInsets .~ .init(all: Styles.grid(2))
+    |> UIButton.lens.titleLabel .. UILabel.lens.font .~ UIFont.boldSystemFont(ofSize: 16)


Lenses give us access to the font property of a UIButton. So this line could simply be |> UIButton.lens.titleLabel.font .~ UIFont.boldSystemFont(ofSize: 16) (this syntax is the most commonly used on our codebase)

Had to update Prelude so if you can also check it out and approve if everything looks fine - kickstarter/Kickstarter-Prelude#95

dusi · 2019-08-20T19:01:55Z

Thanks for catching up the payment methods issue (I've fixed it and went through all the places we refer to base style in the whole app so now all buttons should in fact not support Dynamic Type 🤞 ).

As for the pragma mark I'd defer this to some future work (it would be great to have a whole team agree on this before changing something we've been doing for some time now. // MARK: - Bind Styles is currently only used in 1 place (PledgePaymentMethodsViewController) and I'm kinda tempted at changing it to Styles for consistency 😅

justinswart

Nice, thanks for all the screenshots!

Scollaco

lgtm!

dusi · 2019-08-20T20:20:02Z

Fun fact, those screenshots did actually not fit on my Desktop (I think I need 15" MBP or the new Pro Display soon) 🤦‍♂

Disable dynamic type on the CTA button

59cd9aa

dusi assigned Scollaco Aug 19, 2019

dusi requested review from justinswart and Scollaco August 19, 2019 22:13

dusi commented Aug 19, 2019

View reviewed changes

dusi added the needs review label Aug 19, 2019

justinswart reviewed Aug 19, 2019

View reviewed changes

Simplify button styles

68b887c

justinswart reviewed Aug 20, 2019

View reviewed changes

dusi added 4 commits August 19, 2019 18:07

Replace enum accessor with green button style function

84baa40

Remove ctaButtonStyle

6932aee

Tweak multiline button style

0700aa4

Format code

5e03373

dusi changed the title ~~♿️ Disable Dynamic Type on the CTA button~~ ♿️ Disable Dynamic Type on buttons Aug 20, 2019

Scollaco reviewed Aug 20, 2019

View reviewed changes

dusi added 3 commits August 20, 2019 12:06

Extract styles to private functions

ffd72c6

Disable dynamic type on payment method select button

25b3237

Cleanup button styles

9ba819f

justinswart approved these changes Aug 20, 2019

View reviewed changes

Remove intermediate style

1723d68

Scollaco approved these changes Aug 20, 2019

View reviewed changes

dusi added 2 commits August 20, 2019 13:37

Merge branch 'master' into a11y-cta-button-disable-dynamic-type

2bf9ebd

Point prelude dependency to the latest commit

9df465d

dusi added 2 commits August 20, 2019 13:46

Merge branch 'master' into a11y-cta-button-disable-dynamic-type

454d3c8

Merge branch 'master' into a11y-cta-button-disable-dynamic-type

137b4e4

dusi merged commit 861ea22 into master Aug 20, 2019

dusi deleted the a11y-cta-button-disable-dynamic-type branch August 20, 2019 21:26

justinswart mentioned this pull request Jun 11, 2021

[NT-2030] Copy Strings Readable All Languages #1514

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

♿️ Disable Dynamic Type on buttons #806

♿️ Disable Dynamic Type on buttons #806

dusi commented Aug 19, 2019 •

edited by Scollaco

Loading

dusi Aug 19, 2019

justinswart Aug 20, 2019

dusi Aug 19, 2019

Scollaco Aug 20, 2019

dusi Aug 19, 2019

dusi Aug 19, 2019

dusi Aug 19, 2019

justinswart Aug 19, 2019

justinswart Aug 20, 2019

Scollaco commented Aug 20, 2019

Scollaco Aug 20, 2019

Scollaco Aug 20, 2019

dusi Aug 20, 2019

Scollaco Aug 20, 2019

dusi Aug 20, 2019 •

edited

Loading

dusi commented Aug 20, 2019

justinswart left a comment

Scollaco left a comment

dusi commented Aug 20, 2019

♿️ Disable Dynamic Type on buttons #806

♿️ Disable Dynamic Type on buttons #806

Conversation

dusi commented Aug 19, 2019 • edited by Scollaco Loading

📲 What

🤔 Why

🛠 How

👀 See

👆 Notes

✅ Acceptance criteria

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Scollaco commented Aug 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dusi Aug 20, 2019 • edited Loading

Choose a reason for hiding this comment

dusi commented Aug 20, 2019

justinswart left a comment

Choose a reason for hiding this comment

Scollaco left a comment

Choose a reason for hiding this comment

dusi commented Aug 20, 2019

dusi commented Aug 19, 2019 •

edited by Scollaco

Loading

dusi Aug 20, 2019 •

edited

Loading