Skip to content
This repository was archived by the owner on Feb 5, 2025. It is now read-only.

Conversation

mindgraffiti
Copy link
Contributor

@mindgraffiti mindgraffiti commented Jul 9, 2020

Ref. #322
Testing PR: wordpress-mobile/WordPress-iOS#14450

In this PR, a new view controller, SiteCredentialsViewController, and the basic UI have been added to the SiteAddress.storyboard. This code should look very similar to the SiteAddressViewController, as they share several of the same methods, UI, and IBOutlets.

To test

  1. Check out this branch
  2. bundle exec pod install
  3. Build and run without errors

@mindgraffiti mindgraffiti added the enhancement New feature or request label Jul 9, 2020
@mindgraffiti mindgraffiti self-assigned this Jul 9, 2020
<?xml version="1.0" encoding="UTF-8"?>
<Scheme
LastUpgradeVersion = "1130"
LastUpgradeVersion = "1150"
Copy link
Contributor

Choose a reason for hiding this comment

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

Uh... what dis?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It updates the pods project file to the latest version. It should fix this warning:
Screen Shot 2020-07-09 at 1 15 41 PM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried it again. This doesn't actually fix the warning. Will revert.

Copy link
Contributor

@ScoutHarris ScoutHarris left a comment

Choose a reason for hiding this comment

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

There is a grey area at the bottom of the view. It looks like maybe the view doesn't extend to the bottom?

Site Address Site Credentials
site_addr site_creds

guard let vc = LoginSelfHostedViewController.instantiate(from: .login) else {
DDLogError("Failed to navigate from LoginEmailViewController to LoginSelfHostedViewController")
guard let vc = SiteCredentialsViewController.instantiate(from: .siteAddress) else {
DDLogError("Failed to navigate from SiteAddressViewController to SiteAddressViewController")
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this should be:
DDLogError("Failed to navigate from SiteAddressViewController to SiteCredentialsViewController")

/// Configure the instruction cell.
///
func configureInstructionLabel(_ cell: TextLabelTableViewCell) {
cell.configureLabel(text: "Enter your account information for pamelanguyen.com", style: .body)
Copy link
Contributor

Choose a reason for hiding this comment

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

Per the design, there is a period at the end of this sentence.

@mindgraffiti mindgraffiti requested a review from ScoutHarris July 9, 2020 20:46
@mindgraffiti
Copy link
Contributor Author

@ScoutHarris ready for a second look!

The background color override wasn't in place yet. I added it:

Copy link
Contributor

@ScoutHarris ScoutHarris left a comment

Choose a reason for hiding this comment

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

LGTM!

:shipit:

Copy link
Contributor

@Gio2018 Gio2018 left a comment

Choose a reason for hiding this comment

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

Looks good, I just had one comment.

/// Rows listed in the order they were created.
///
enum Row {
case instructions
Copy link
Contributor

Choose a reason for hiding this comment

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

just a thought: unless you are going to have a considerable number of different cases, maybe you could just refer to the constant (TextLabelTableViewCell.reuseIdentifier) since it's already accessible from the same scope?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are going to be 5 cases in total, so this enum will expand quite a bit in the next PR. I put the instruction row in so that it proves the table is displaying correctly and populating data.

@mindgraffiti
Copy link
Contributor Author

Thanks @ScoutHarris and @Gio2018!

@mindgraffiti mindgraffiti merged commit 18840ec into develop Jul 9, 2020
@mindgraffiti mindgraffiti deleted the feature/322-site-address-username-password branch July 9, 2020 21:13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants