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

Conversation

@selanthiraiyan
Copy link
Contributor

@selanthiraiyan selanthiraiyan commented Aug 23, 2022

For woocommerce/woocommerce-ios#7553

Changes

In order to open custom web pages for login-related screens, we need to be able to identify the login screens from WC iOS. I decided to use the WordPressSupportSourceTag to identify each login screen.

  • Adds a WordPressSupportSourceTag to SiteAddressViewController to identify the screen in WC iOS.
  • Adds Equatable conformance to WordPressSupportSourceTag in order to equate the values in WC iOS.

Testing steps

Follow the testing instructions from woocommerce/woocommerce-ios#7553

Copy link
Contributor

@jaclync jaclync left a comment

Choose a reason for hiding this comment

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

LGTM after the pod version update 👍

Pod::Spec.new do |s|
s.name = 'WordPressAuthenticator'
s.version = '2.3.0'
s.version = '2.3.1-beta.1'
Copy link
Contributor

Choose a reason for hiding this comment

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

As Oguz @ Platform9 mentioned in the last code freeze #670 (comment), we want to follow the version scheme by bumping the middle digit for a new release. The last digit is only for hotfix.

Suggested change
s.version = '2.3.1-beta.1'
s.version = '2.4.0-beta.1'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for sharing this. 🙏
Done in dcd1691

Comment on lines 15 to 17
get {
.loginSiteAddress
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: could it be simplified:

Suggested change
get {
.loginSiteAddress
}
.loginSiteAddress

it's also interesting that the loginSiteAddress case exists before but doesn't seem to be used before this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Simplified in 9ea47ec

it's also interesting that the loginSiteAddress case exists before but doesn't seem to be used before this PR?

It is used here, and it seems like the screen isn't being used anymore. I believe that this screen and other screens from that flow were deprecated after the unified login project. Internal ref - p91TBi-2FR-p2

@selanthiraiyan
Copy link
Contributor Author

Thanks for the review @jaclync!

As we decided to use Step and Flow for identification and tracking purposes I opened a new PR, and I am closing this now.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants