Skip to content

Conversation

@momo-ozawa
Copy link
Contributor

@momo-ozawa momo-ozawa commented Dec 5, 2023

Description

  • Updates the My Site header to show site actions in a menu
    • pbArwn-6kb-p2#comment-7901
    • p1701711667406509/1701702771.100599-slack-C04SFL0RP51
  • Updates "Open in Browser" menu option to "Visit site" for consistency
  • Note: I'm using SFSymbols for the menu icons because system images are adaptive. I tried to find the most relevant icon for the action - happy to switch it out for a different icon if needed @osullivanchris, @kean
Before After

How to test

  • Go to My Site
  • Tap on the ellipsis button in the site header
  • Verify you see the following options
    • Visit site
    • Switch site
    • Change site title
    • Change site icon
    • Personalize home
  • Verify each option works as expected
Simulator.Screen.Recording.-.iPhone.15.Pro.-.2023-12-05.at.11.29.45.mp4

Regression Notes

  1. Potential unintended areas of impact
    switching sites

  2. What I did to test those areas of impact (or what existing automated tests I relied on)
    updated UI test

  3. What automated tests I added (or what prevented me from doing so)
    see above

PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding unit tests for my changes.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary. (Improved My Site header: Update release notes #22203)

return DDLogError("Failed to show dashboard personalization screen: siteID is missing")
}

// TODO: track event
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also need to track when the ellipsis button is tapped. I'll add these events in a separate PR

Copy link
Contributor

@kean kean Dec 5, 2023

Choose a reason for hiding this comment

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

I needed this recently too and learned about UIControlEventMenuActionTriggered.

@osullivanchris
Copy link

Looks great so far! WDYT about adding 16px right padding to the right of the menu icon? It will be aligned more nicely with the chevrons in the list below.

@osullivanchris
Copy link

Actually just checked my mock and I had 8 left and 12 right
image

@momo-ozawa
Copy link
Contributor Author

@osullivanchris Done!

@momo-ozawa momo-ozawa force-pushed the hack/site-header-context-menu branch from a7e71d2 to 3023b84 Compare December 5, 2023 15:21
private func makePrimarySection() -> UIMenu {
UIMenu(options: .displayInline, children: [
MenuItem.visitSite(visitSiteTapped),
MenuItem.switchSite(siteSwitcherTapped)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should it only be displayed if there is more than one site?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I think we should always show it - to add a new site you need to access the My Sites screen

Choose a reason for hiding this comment

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

Yes I agree. The action should always be there in the menu. but the 'tap icon to switch site' idea - would only be for multiple sites I think. If we do that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Tapping "Switch Site" is not the most obvious way to add a new site. I'd suggest adding two new actions "Create Site" and "Share" to the new menu.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair point. I'll look into this after the header gradient.

@kean
Copy link
Contributor

kean commented Dec 5, 2023

Tested – works great! The actions are much more discoverable now. I left one comment regarding "Switch Site". It was smart not to update the current iterations: tap on an icon, title etc, so that we could keep Quick Start as is for now.

Copy link

@osullivanchris osullivanchris left a comment

Choose a reason for hiding this comment

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

Looks great! thanks for the padding tweak.

@wpmobilebot
Copy link
Contributor

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 Numberpr22151-a453c37
Version23.8
Bundle IDorg.wordpress.alpha
Commita453c37
App Center BuildWPiOS - One-Offs #8045
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@wpmobilebot
Copy link
Contributor

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 Numberpr22151-a453c37
Version23.8
Bundle IDcom.jetpack.alpha
Commita453c37
App Center Buildjetpack-installable-builds #7066
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@osullivanchris
Copy link

Just installed the build and it feels massively better and more modern in hand compared to the live app 👏

@kean kean self-requested a review December 5, 2023 21:14
@momo-ozawa momo-ozawa merged commit f5285e8 into trunk Dec 6, 2023
@momo-ozawa momo-ozawa deleted the hack/site-header-context-menu branch December 6, 2023 11:31
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.

5 participants