Make remaining colors configurable #107
Conversation
- now that WPiOS provides a style, defaultStyle seems unnecessary
|
Heya @nheagy ! Looking good from here. I think CI is being cranky about this warning: Thoughts? |
|
@aerych I hadn't noticed that. Pushed another commit to move |
aerych
left a comment
aerych
left a comment
There was a problem hiding this comment.
Shiny! Retested and the warning is gone. Code looks good. 
from me.
|
@aerych I pushed one more commit as I found some more colors to change in the signup epilogue. Care to give it a glance? |
|
Hard to not miss a few strays given the amount of code here. :) Its all good. My |
jaclync
left a comment
jaclync
left a comment
There was a problem hiding this comment.
the changes look good, just a non-blocking nit!
I was testing your branch in WCiOS, and the UI from launch to logging in with username/password/2FA looks great (we just need to specify the new color arguments). However, I wasn't able to test logging in by entering site address. Something between version 1.5.2 and 1.5.3 seems to make it not work:
CredStore - performQuery - Error copying matching creds. Error=-50, query={
"m_Limit" = "m_LimitAll";
ptcl = htps;
srvr = "funtestingusa.wpcomstaging.com";
sync = syna;
}
were there any changes we had to make when updating the WordPressAuthenticator pod from < 1.5.3 to > 1.5.3 on WPiOS?
| .foregroundColor: WordPressAuthenticator.shared.style.placeholderColor, | ||
| .font: font | ||
| ] | ||
| self.attributedPlaceholder = NSAttributedString(string: placeholder, attributes: attributes) |
There was a problem hiding this comment.
nit: self might not be necessary?
- also makes it more reliable, previously it was only colored once and if changed it reverted to iOS default (which was happening for unknown reasons)
|
Thanks for your reviews! Does one of you have time for a small re-review? @jaclync based on your prod to fix |
jaclync
left a comment
jaclync
left a comment
There was a problem hiding this comment.
just a question about the color change and a non-blocking nit, otherwise LGTM!
| blavatarImageView.layer.borderColor = WordPressAuthenticator.shared.style.instructionColor.cgColor | ||
| blavatarImageView.layer.borderWidth = 1 | ||
| blavatarImageView.tintColor = WPStyleGuide.greyLighten10() | ||
| blavatarImageView.tintColor = WordPressAuthenticator.shared.style.instructionColor |
There was a problem hiding this comment.
the old diffs seem to have different shades of grey for the border and tint, and the new diffs seem to have the same - just double checking this is an intentional change?
| self.secondaryTitleColor = secondaryTitleColor | ||
| self.disabledTitleColor = disabledTitleColor | ||
| self.textButton = textButton | ||
| self.textButtonHighlight = textButtonHighlight |
There was a problem hiding this comment.
sorry didn't comment on this earlier, a non-blocking nit: maybe buttonTextColor/buttonTextHighlightedColor would sound more clear that these two are colors not buttons? also more consistent with the names of other arguments (same for the property names)
There was a problem hiding this comment.
I agree, these should be suffixed with Color.
This whole struct has gotten a bit big for my tastes, but I'll leave a more thorough refactor for another time 😉
|
@jaclync great suggestions, now fixed! |
…-changes Make remaining colors configurable
To finish my efforts to use the new WPiOS color scheme during login, this PR makes the remaining colors from the old WPiOS color palette configurable. It also removes the default
WordPressAuthenticatorStyle, and makes thestyleargument mandatory.This will have compatibility implications for the other projects that use this pod.
Examples: