NUXButton configurable using UIAppearance #10

jleandroperez · 2018-07-05T17:43:01Z

Not sure how the former one got closed (apparently, happened automatically after i've deleted new-woo-apis).

I'm pasting, below, the former PR's description.

⚠️ The secondary button's default configurations have not been tested against the original button designs for WPiOS. Since it's branched from new-woo-apis it isn't compatible with WPiOS just yet and was breaking when I attempted to test the secondary button. fully tested and compatible ✅

This PR makes the NUXButton configurable using UIAppearance. To run a full test, check out the WCiOS PR instructions.

…nges like `FancyButton` does

Removes NUXSubmitButton class. Removes button background assets and generates button background images.

… consistent) in NUXButton

…-appearance

jleandroperez

@mindgraffiti Few comments, looking great!

jleandroperez · 2018-07-05T17:44:57Z

WordPressAuthenticator/NUX/NUXButton.swift

+            var frm = activityIndicator.frame
+            frm.origin.x = (frame.width - frm.width) / 2.0
+            frm.origin.y = (frame.height - frm.height) / 2.0
+            activityIndicator.frame = frm


activityIndicator.frame = frm.integral

Reason: floating positions don't really exist (the screen requires integer coordinates). Anything that has a comma value results in a blurry view.

cc @mindgraffiti

This is confusing. x, y, and frame all accept CGFloat values. Why do they make it like that if they really want ints?

@mindgraffiti actually, my explanation was wrong. Lemme rephrase: positions are expressed in terms of Points, not Pixels. The problem is:

Non Retina Devices: PositionX = 0.5 maps to half a pixel in the X axis

@2x Devices: PositionX = 0.5 maps to one pixel

@3x Devices: Position X = 0.5 maps to one pixel and a half

Any "Pixel Position" that's not integral, when rendered, gets antialias applied. Which is why you usually wanna avoid non integral positions (because they may look blurry).

Exception is: when you're actually calculating things, so it maps to integral pixels. Sometimes you may wanna draw a line 1 pixel width, and there are super simple ways to do so.

A little bit more about this here.

jleandroperez · 2018-07-05T17:46:01Z

WordPressAuthenticator/NUX/NUXButton.swift

+        static let normalBackgroundColor = UIColor.white
+        static let normalBorderColor = WPStyleGuide.greyLighten20()
+        static let highlightBackgroundColor = WPStyleGuide.greyLighten20()
+        static let highlightBorderColor = highlightBackgroundColor


@mindgraffiti shouldn't all of the values here come from the new Style struct? (Primary.*Color / Secondary.*Color)

jleandroperez · 2018-07-05T17:46:19Z

WordPressAuthenticator/NUX/NUXButton.swift

+    struct Title {
+        static let primaryColor = UIColor.white
+        static let secondaryColor = WPStyleGuide.darkGrey()
+        static let disabledColor = WPStyleGuide.greyLighten30()


Same comment as above: Shouldn't these come from the new Style struct?

jleandroperez · 2018-07-05T17:49:12Z

WordPressAuthenticator/Authenticator/WordPressAuthenticator.swift

+
+    /// Alternative logins highlight color
+    ///
+    let highlightColor: UIColor


Please set linkColor and highlightColor as public, otherwise they won't be accessible from outside the framework.

jleandroperez · 2018-07-05T17:49:27Z

WordPressAuthenticator/Authenticator/WordPressAuthenticator.swift

+}
+
+public extension WordPressAuthenticatorStyle {
+    static var defaultStyle: WordPressAuthenticatorStyle {


@mindgraffiti please set to public

jleandroperez · 2018-07-05T17:49:54Z

WordPressAuthenticator/Authenticator/WordPressAuthenticator.swift

    /// Initializes the WordPressAuthenticator with the specified Configuration.
    ///
-    public static func initialize(configuration: WordPressAuthenticatorConfiguration) {
+    public static func initialize(configuration: WordPressAuthenticatorConfiguration, style: WordPressAuthenticatorStyle) {


@mindgraffiti Can we have style: WordPressAuthenticatorStyle = .defaultStyle ?

jleandroperez · 2018-07-05T17:50:53Z

WordPressAuthenticator/NUX/NUXButton.swift

+import WordPressUI
+
+/// A protocol for an element that can display a UIActivityIndicatorView
+@objc public protocol ActivityIndicatorButton {


@mindgraffiti since the protocol ActivityIndicatorButton isn't really used anywhere (there are no ivars / vars anywhere with that specific Type), can we nuke it?

jleandroperez · 2018-07-05T17:51:54Z

WordPressAuthenticator/NUX/NUXButton.swift

-    fileprivate let maxFontSize: CGFloat = 22
+@objc open class NUXButton: UIButton, ActivityIndicatorButton {
+    @objc var isAnimating: Bool {
+        get {


@mindgraffiti you can drop the get { } block. If a property is readOnly, you can just do something like this:

var isAnimating: Bool { return activityIndicator.isAnimating }

nheagy · 2018-07-05T18:49:05Z

Podfile.lock

@@ -1,20 +1,5 @@
 PODS:
  - 1PasswordExtension (1.8.5)
-  - AFNetworking (3.2.1):


Any idea why the Podfile changes so much without Podfile changes?

Yes, that's due to all the mucking about I did with making some of the pods local. I think I need to pull the original Podfile and Podfile.lock and overwrite the changes made in my PR.

We've been working on nuking AFNetworking (with Sergio) during last Hack Week, from WordPressKit. This change is just (or should be!!) a side effect of running pod install.

No need to bring back the old Podfile.lock, this is a beautiful thing actually!!! (one less dependency).

Ah, got it. 👍

nheagy

I'm really interested to see how these work in WPiOS—code cleanup in that area sounds great.

I think we should holdoff on merging this until the WPiOS implementation is available. Let me know if you think otherwise.

nheagy · 2018-07-05T18:50:28Z

WordPressAuthenticator/NUX/NUXButton.swift

 /// A stylized button used by Login controllers. It also can display a `UIActivityIndicatorView`.
-@objc open class NUXButton: NUXSubmitButton {
-    // MARK: - Configuration
-    fileprivate let horizontalInset: CGFloat = 20


Where'd these config values go?

They moved to here: https://github.com/wordpress-mobile/WordPressUI-iOS/blob/develop/WordPressUI/Extensions/UIImage%2BAssets.swift#L87

The NUXButton was using the same sizing and configurations as FancyButton, so I moved the definitions out of FancyButton and into the UIImage+Assets extension. This PR has more details wordpress-mobile/WordPressUI-iOS#8

mindgraffiti · 2018-07-05T19:41:42Z

I think we should holdoff on merging this until the WPiOS implementation is available. Let me know if you think otherwise.

Agreed! I want to thoroughly test this now that the new-woo-apis branch has been integrated. Was looking for some initial impressions on the work so far, to make sure I'm on the right track.

…to their own files. Rely on WordPressAuthenticatorStyle defaults instead of styling NUXButton directly

jleandroperez

@mindgraffiti looks great!! few nipticky comments. I'll get to testing the WCiOS Integration PR!!

jleandroperez · 2018-07-06T13:27:19Z

WordPressAuthenticator/Extensions/WPStyleGuide+Login.swift

        super.traitCollectionDidChange(previousTraitCollection)
        if previousTraitCollection?.preferredContentSizeCategory != traitCollection.preferredContentSizeCategory {
            titleLabel?.font = WPStyleGuide.mediumWeightFont(forStyle: .subheadline)
+            titleLabel?.textColor = WPStyleGuide.wordPressBlue()


@mindgraffiti since Woo is using this, shouldn't we probably grab this one from the Style struct?

jleandroperez · 2018-07-06T14:10:56Z

Podfile.lock

@@ -1,20 +1,5 @@
 PODS:
  - 1PasswordExtension (1.8.5)
-  - AFNetworking (3.2.1):


We've been working on nuking AFNetworking (with Sergio) during last Hack Week, from WordPressKit. This change is just (or should be!!) a side effect of running pod install.

No need to bring back the old Podfile.lock, this is a beautiful thing actually!!! (one less dependency).

jleandroperez · 2018-07-06T14:11:29Z

WordPressAuthenticator/Authenticator/WordPressAuthenticator.swift

-    }
-}
-
-


❤️ this change!!

jleandroperez · 2018-07-06T14:14:10Z

WordPressAuthenticator/NUX/NUXButton.swift

+        setBackgroundImage(highlightedImage, for: .highlighted)
        setBackgroundImage(disabledImage, for: .disabled)

+        activityIndicator.activityIndicatorViewStyle = .gray


Perhaps this should go into configureActivityIndicator() (since it's not really a Background?)

jleandroperez · 2018-07-06T14:16:20Z

WordPressAuthenticator/NUX/NUXButton.swift

+            frm.origin.x = (frame.width - frm.width) / 2.0
+            frm.origin.y = (frame.height - frm.height) / 2.0
+            activityIndicator.frame = frm.integral
+        }


Looks good!! but now that i realize about it: why not just Autolayout? (centerXAnchor / centerYAnchor)

I'm drawing the line here. Need to get this PR closed. :)

…ethod, ... ...use WordPressAuthenticator styles for titleLabel `traitCollectionDidChange`

jleandroperez

@mindgraffiti looking great overall!!. Added just one important comment. Switching to WPiOS testing as of now, will get back to this one in a bit

jleandroperez · 2018-07-08T14:19:27Z

WordPressAuthenticator/NUX/NUXButton.swift


+    /// Setup: shorter reference for style
+    ///
+    private let style = WordPressAuthenticator.shared.style


typealias Style = WordPressAuthenticator.shared.style

Error: Static var 'shared' is not a member type of 'WordPressAuthenticator'

Ahhhhhhhh right, it's not a type!!!. My apologies!!. Only alternative is having this as a calculated property. But "meh", really no difference in there.

Sorry about the fuzz!!

jleandroperez · 2018-07-08T14:19:58Z

WordPressAuthenticator/NUX/NUXButton.swift

    }

-    override open func configureBorderColor() {
+    struct Metrics {


This one should be private

jleandroperez · 2018-07-09T14:17:39Z

WordPressAuthenticator/NUX/NUXButton.swift

+        }
+        configureBackgrounds()
+        configureTitleColors()
+        activityIndicator.activityIndicatorViewStyle = .gray


You probably wanna move these three lines (configureBackgrounds, configureTitleColors and configureActivityIndicator) elsewhere. Perhaps to awakeFromNib.

Reason: Each time you call this, configureBackgrounds will re-add the activityIndicator as a subview. This should probably print a warning or so, but it's something we should run just once.

…nged, only allow color changes.

…Nib()`

nheagy

Ok, @mindgraffiti let's go!

mindgraffiti · 2018-07-10T20:27:02Z

Thank you for helping me out with the NUXButton @nheagy!

mindgraffiti and others added 12 commits July 2, 2018 16:31

Reference updated WordPressUI git branch for UIImage+Asset extension

00440f7

Initial changes to make NUXSubmitButton conform to UIAppearance cha…

d19def4

…nges like `FancyButton` does

Restore podfile settings

f42902c

Makes NUXButton configurable using UIAppearance

524ef64

Removes NUXSubmitButton class. Removes button background assets and generates button background images.

Delete unused image asset methods. Remove extra line (make code style…

44edf1f

… consistent) in NUXButton

Allows all standard UIButton buttons to be styled using UIAppearance

c10d3d0

Restore podfile.lock settings

89cc7b5

Pod Update

807364b

Change Metric to DefaultRenderMetrics

2fd7815

Add a singleton with Auth styles

e5ac7a0

Finish authenticator style implementation for alternative login links

2edecde

Merge remote-tracking branch 'origin/develop' into feature/nux-button…

1fef7ae

…-appearance

jleandroperez commented Jul 5, 2018

View reviewed changes

jleandroperez requested a review from mindgraffiti July 5, 2018 17:47

jleandroperez added the enhancement New feature or request label Jul 5, 2018

jleandroperez commented Jul 5, 2018

View reviewed changes

nheagy reviewed Jul 5, 2018

View reviewed changes

mindgraffiti self-assigned this Jul 5, 2018

mindgraffiti added 4 commits July 5, 2018 15:01

Code review - use integral

e1d3781

Fix error when attempting to build for WCiOS

16c4724

Move structs and classes out of WordPressAuthenticator.swift and in…

eeba61e

…to their own files. Rely on WordPressAuthenticatorStyle defaults instead of styling NUXButton directly

Delete rogue line

6a31dea

jleandroperez commented Jul 6, 2018

View reviewed changes

Add missing import. Make var public.

3ed9b8b

mindgraffiti added 4 commits July 6, 2018 10:48

Fix assignment in init method

87e916f

Code review changes - move activity indicator setting to its config m…

b29fc12

…ethod, ... ...use WordPressAuthenticator styles for titleLabel `traitCollectionDidChange`

Use shared style instead of defaultStyle

db510d5

Set the titleFont for defaultstyle

abaffb0

mindgraffiti mentioned this pull request Jul 6, 2018

Support new WordPressAuthenticator configuration and API methods wordpress-mobile/WordPress-iOS#9714

Merged

jleandroperez commented Jul 9, 2018

View reviewed changes

mindgraffiti added 4 commits July 9, 2018 11:02

Set a fixed font style for NUXButton titleLabel. Leave the font uncha…

2041ab6

…nged, only allow color changes.

Re-set NUXButton titleLabel font to original settings

654c331

Update comment

7d703e5

Code review changes - move activityIndicator view style to `awakeFrom…

e730a89

…Nib()`

mindgraffiti removed their request for review July 9, 2018 17:10

mindgraffiti added 5 commits July 9, 2018 14:04

Merge branch 'develop' into feature/nux-button-appearance

5bb5775

Remove shadowOffset and use default set in WordPressUI

695d8c1

Change font settings for NUXButton

046625d

Temporarily point to WordPressUI branch under active development

ad03b31

Restore podfile to point to WordPressUI ~> 1.0 version

74e297b

nheagy approved these changes Jul 10, 2018

View reviewed changes

mindgraffiti merged commit 0554279 into develop Jul 10, 2018

mindgraffiti deleted the feature/nux-button-appearance branch July 10, 2018 20:26

NUXButton configurable using UIAppearance #10

NUXButton configurable using UIAppearance #10

Uh oh!

Conversation

jleandroperez commented Jul 5, 2018 • edited by mindgraffiti Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jleandroperez left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jleandroperez Jul 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jleandroperez Jul 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jleandroperez Jul 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nheagy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mindgraffiti commented Jul 5, 2018

Uh oh!

jleandroperez left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jleandroperez commented Jul 5, 2018 •

edited by mindgraffiti

Loading

jleandroperez Jul 5, 2018 •

edited

Loading

jleandroperez Jul 5, 2018 •

edited

Loading

jleandroperez Jul 5, 2018 •

edited

Loading