Color updates: update text button title color and configure alternative button in LoginEmailViewController
#164
Conversation
frosty
left a comment
frosty
left a comment
There was a problem hiding this comment.
Thanks for updating this! I checked out WPiOS, and I think the new color is correct – it looks like this was a case where we forgot to update things when we changed the color scheme :)
I added a couple of comments about code organisation – most of them are just the same comment but for different classes.
|
|
||
| extension UIButton { | ||
| /// Applies default styles to a plain text button. | ||
| func applyTextButtonStyle() { |
There was a problem hiding this comment.
I think this method might be better suited added to WPStyleGuide instead of UIButton. You could add it to WPStyleGuide+Login.swift, which is where we have methods to configure other buttons like the 1Password button (you may also want to rename it to configureTextButton instead of applyStyle as that might be more consistent).
| sendCodeButton.setTitle(NSLocalizedString("Text me a code instead", comment: "Button title"), | ||
| for: .normal) | ||
| sendCodeButton.titleLabel?.numberOfLines = 0 | ||
| sendCodeButton.applyTextButtonStyle() |
There was a problem hiding this comment.
I don't feel like this call belongs in a method named localizeControls, which is all around localising and setting text. We could just create a new configureTextButton method to include this call?
| forgotPasswordButton.setTitle(forgotPasswordTitle, for: .normal) | ||
| forgotPasswordButton.setTitle(forgotPasswordTitle, for: .highlighted) | ||
| forgotPasswordButton.titleLabel?.numberOfLines = 0 | ||
| forgotPasswordButton.applyTextButtonStyle() |
There was a problem hiding this comment.
As above, I don't think this belongs in the localize method.
| siteAddressHelpButton.setTitle(siteAddressHelpTitle, for: .normal) | ||
| siteAddressHelpButton.setTitle(siteAddressHelpTitle, for: .highlighted) | ||
| siteAddressHelpButton.titleLabel?.numberOfLines = 0 | ||
| siteAddressHelpButton.applyTextButtonStyle() |
| forgotPasswordButton.setTitle(forgotPasswordTitle, for: .normal) | ||
| forgotPasswordButton.setTitle(forgotPasswordTitle, for: .highlighted) | ||
| forgotPasswordButton.titleLabel?.numberOfLines = 0 | ||
| forgotPasswordButton.applyTextButtonStyle() |
| forgotPasswordButton?.setTitle(forgotPasswordTitle, for: .normal) | ||
| forgotPasswordButton?.setTitle(forgotPasswordTitle, for: .highlighted) | ||
| forgotPasswordButton?.titleLabel?.numberOfLines = 0 | ||
| forgotPasswordButton?.applyTextButtonStyle() |
…StyleGuide+Login`. Update existing usage and configure the "Enter your password instead." text button in `LoginLinkRequestViewController`.
|
@frosty thanks for checking the app and the fast review! updated the PR based on your feedback 😄 |
Changes
UIButtonextension to style a text button usingWordPressAuthenticator.shared.style.textButtonColorfor normal color andWordPressAuthenticator.shared.style.textButtonHighlightColorfor highlighted colorLoginEmailViewController, calledconfigureAlternativeLabelinviewWillAppearalong with configurations for other subviewsWCiOS PR
This is the WCiOS PR that requires the color updates in this PR: woocommerce/woocommerce-ios#1565
WPiOS changes
Example branch:
issue/wcios-1555-color-updatesI noticed a slight color change in the text button, I'm not sure if the color from the xib is the one to be used for
textButtonColorstyle parameter?