Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Domain Management] Add My Domains to Me Screen #21701

Merged
merged 8 commits into from
Oct 24, 2023

Conversation

alpavanoglu
Copy link
Contributor

@alpavanoglu alpavanoglu commented Oct 6, 2023

Refs #21424
Refs #21425

This PR adds the "My Domains" tab to "Me" screen and tapping on the button will open the empty view controller.

Testing Steps

  1. Install & Login Jetpack App
  2. RemoteFeatureFlag domainManagement default value on line 63. needs to be set to true.
  3. Go to "Me"
  4. Verify if a "My Domains" tab appears under the new "Products" section. (The tab will be made shorter to match the designs on a separate PR).
  5. Tap on the button and verify if a new screen is presented.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Oct 6, 2023

WordPress Alpha📲 You can test the changes from this Pull Request in WordPress Alpha by scanning the QR code below to install the corresponding build.
App NameWordPress Alpha WordPress Alpha
ConfigurationRelease-Alpha
Build Numberpr21701-0084f27
Version23.5
Bundle IDorg.wordpress.alpha
Commit0084f27
App Center BuildWPiOS - One-Offs #7512
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Oct 6, 2023

Jetpack Alpha📲 You can test the changes from this Pull Request in Jetpack Alpha by scanning the QR code below to install the corresponding build.
App NameJetpack Alpha Jetpack Alpha
ConfigurationRelease-Alpha
Build Numberpr21701-0084f27
Version23.5
Bundle IDcom.jetpack.alpha
Commit0084f27
App Center Buildjetpack-installable-builds #6539
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@alpavanoglu alpavanoglu added this to the 23.6 milestone Oct 20, 2023
@alpavanoglu alpavanoglu marked this pull request as ready for review October 20, 2023 11:16
@alpavanoglu alpavanoglu changed the title [Domain Management] Add My Domains to Me & UIMenu [Domain Management] Add My Domains to Me Screen Oct 24, 2023
import UIKit

final class MyDomainsViewController: UIViewController {
enum Action: String {
Copy link
Contributor

Choose a reason for hiding this comment

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

@alpavanoglu This enum should be removed. right?

Copy link
Contributor

@salimbraksa salimbraksa left a comment

Choose a reason for hiding this comment

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

LGTM!

@alpavanoglu alpavanoglu merged commit 47e98ba into trunk Oct 24, 2023
@alpavanoglu alpavanoglu deleted the 21425-my-domains-and-menu branch October 24, 2023 09:38
Copy link
Contributor

@hassaanelgarem hassaanelgarem left a comment

Choose a reason for hiding this comment

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

I tested this on a simulator and I noticed the following issue. The new button doesn't match the design. The text is centered, and it is missing the icon. Also, there's a "Products" section header that is not available in the design.
Design ref: rvVgXupiyONUcJMgPSoMFj-fi-2565_27886

PR Build Design
Simulator Screenshot - iPhone 14 Pro Max - 2023-10-25 at 01 17 10 iPhone 19

Edit: I just noticed you mentioned inside the testing steps there's another PR to follow this 👍

}())
]

#if JETPACK
Copy link
Contributor

Choose a reason for hiding this comment

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

[Question] Why are we using macros here? Have you considered using AppConfiguration.isJetpack?

Copy link
Contributor

@salimbraksa salimbraksa Oct 24, 2023

Choose a reason for hiding this comment

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

I can answer this question as it was my suggestion @hassaanelgarem.

Without the macro, the MyDomainsViewController files and all the files that depend on it would have to be added to WordPress target too. Even though the My Domains screen is for Jetpack only.

And if we remove those files from WordPress target, then the WordPress app won't build.

@alpavanoglu considered using isJetpack flag but that doesn't fix this problem.

@salimbraksa
Copy link
Contributor

salimbraksa commented Oct 24, 2023

@hassaanelgarem Based on the "My Domains" design, it looks like the row doesn't have the disclosure indicator ( chevron icon ), which indicates that the screen should be presented modally.

As I said earlier, I'm not a fan of presenting that screen modally. So what do you say we add the chevron like other rows?

Also, that globe icon doesn't resemble the other icons. 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants