Skip to content

Conversation

@jleandroperez
Copy link
Contributor

@jleandroperez jleandroperez commented May 28, 2018

Details:

This PR implements the Login Epilogue UI: We'll display a Store Picker, which will allow the user(s) to select the active WooCommerce Store.

I'm splitting this PR in two, since i'm already approaching 586 LOC.

Ref. #90

cc @mindgraffiti @bummytime
Thanks in advance!!

Testing:

  1. Verify the Unit Tests are still green!
  2. Perform a fresh install
  3. Log into your account

Verify that the Login Epilogue should show up. Note that at this stage, the Epilogue is expected to look as follows (no matter if you do have or do not have a Store setup). To be finished in a second PR.

simulator screen shot - iphone se - 2018-07-10 at 19 40 16


static var defaultTextColor: UIColor {
return active.defaultTextColor
static var tableViewBackgroundColor: UIColor {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opening Brace Spacing Violation: Opening braces should be preceded by a single space and on the same line as the declaration. (opening_brace)

/// White-Background View, to be placed surrounding the bottom area.
///
@IBOutlet private var backgroundView: UIView! {
didSet {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opening Brace Spacing Violation: Opening braces should be preceded by a single space and on the same line as the declaration. (opening_brace)


static var buttonPrimaryColor: UIColor {
return active.buttonPrimaryColor
static var tableViewBackgroundColor: UIColor {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opening Brace Spacing Violation: Opening braces should be preceded by a single space and on the same line as the declaration. (opening_brace)

}

func downloadGravatarImage() {
guard let targetURL = StoresManager.shared.sessionManager.defaultAccount?.gravatarUrl, let gravatarURL = URL(string: targetURL) else {
Copy link
Collaborator

@wpmobilebot wpmobilebot Jul 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Opening braces should be preceded by a single space and on the same line as the declaration.


/// AccountHeaderView: Displays an Account's Details: [Gravatar + Name + Username]
///
class AccountHeaderView: UIView {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Opening braces should be preceded by a single space and on the same line as the declaration.

@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Jul 10, 2018

1 Warning
⚠️ PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 Danger

set {
fullnameLabel.text = newValue
}
get {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Opening braces should be preceded by a single space and on the same line as the declaration.

@jleandroperez jleandroperez changed the title WIP: Implements Login Epilogue Login Epilogue: Mark I Jul 10, 2018
@jleandroperez jleandroperez added feature: login Related to any part of the log in or sign in flow, or authentication. and removed [Status] Not Ready for Review labels Jul 10, 2018
Copy link
Contributor

@mindgraffiti mindgraffiti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unit tests ✅
Looking good @jleandroperez! Some nitpicky items then :shipit:

///
@IBOutlet private var noResultsLabel: UILabel! {
didSet {
noResultsLabel.font = UIFont.font(forStyle: .subheadline, weight: .regular)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add to StyleManager

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call!!




// MARK: - Overriden Methods
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overridden instead of Overriden

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My GOD you must have sent me that comment 99 times already. SORRY about making the same mistake over and over!!!!

return
}

headerView.username = "@" + defaultAccount.username
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to have a tiny convenience method (maybe in defaultAccount?) that added the @ symbol to a username string, because we'll be using this a lot more in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Holding this one for a bit... because, honestly, i have no idea what to call this calculated getter, and everything sounds awful (all of it)!!!.

"Prefixed Username" ... "At Username"... "usernameForDisplay"

Copy link
Contributor

@bummytime bummytime left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good @jleandroperez. Unit tests are ✅.

One thing I will point out is that the EmptyStoresTableViewCell doesn't render well in landscape:

napkin 76 07-11-18 1 20 55 pm

You may want to disable landscape on this screen like we do on WPiOS.

Your call if you want to address that 👆 in this PR or create a separate issue.

Other than that, :shipit:

@jleandroperez
Copy link
Contributor Author

Thanks for the review @mindgraffiti + @bummytime !!!. Addressing the pendings (Rotation support) in a second PR, which is in the works. For the record, pendings are:

  • Rotation Issues
  • @username calculated property!

@jleandroperez jleandroperez merged commit f31cad4 into develop Jul 11, 2018
@jleandroperez jleandroperez deleted the issue/22-woo-epilogue branch July 11, 2018 19:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature: login Related to any part of the log in or sign in flow, or authentication.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants