-
Notifications
You must be signed in to change notification settings - Fork 826
Admin Menu: Consolidate "Dashboard" and "My Home" menus #43983
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
Conversation
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! |
Code Coverage SummaryCoverage changed in 5 files.
Full summary · PHP report · JS report Coverage check overridden by
I don't care about code coverage for this PR
|
projects/packages/masterbar/src/admin-menu/class-admin-menu.php
Outdated
Show resolved
Hide resolved
projects/packages/jetpack-mu-wpcom/src/features/wpcom-admin-menu/wpcom-admin-menu.php
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works as described
…ing-ia-dashboard-home
Tests well on Simple and Atomic, however, there's a small priority issue on p2s. By moving the My home out of hosting for classic interface, it means the overview would become the default page. Not a problem, but it would be interesting to see what's the impact of this (more domains, more staging sites usage, etc) since in theory it would receive more exposure. |
Actually, it's not only P2s, but all sites. Great catch. And like you said, not a problem, but would be nice if as a side effect of this change users are more aware of the Hosting Dashboard. |
Sorry, I should've been more clear. That comment is about two separate things :D
|
Got it, thanks for clarifying! Just pushed a fix for that, along with another fix to prevent the "My Home" menu from being visible to super admins who are not members of the site (and thus without Calypso access for that site). |
projects/packages/jetpack-mu-wpcom/src/features/wpcom-admin-menu/wpcom-admin-menu.php
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 🚢
…ing-ia-dashboard-home
- Stops replacing the "Dashboard" menu with "My Home". This way, the "Dashboard" menu is consistently visible across all sites (previously, only visible with the classic admin interface). - Always registers "My Home" as a top-level menu, so it's consistently in the same level across all sites (previously, it was a submenu of "Hosting" on sites with the classic admin interface).
Fixes DOTCOM-13618 and DOTCOM-13312
Proposed changes:
Note that the "Dashboard > Updates" submenu is still not visible on Simple sites, so the "Dashboard" menu won't display any submenus. In a follow-up (see DOTCOM-13619), we'll add "Dashboard > Updates" to Simple sites, and this will cause the "Dashboard > Home" to show up.
Other information:
Jetpack product discussion
N/A
Does this pull request change what data or activity we track or use?
No
Testing instructions: