Admin menu: Register Calypso settings pages as independent submenus #20100

mmtr · 2021-06-17T15:08:30Z

Changes proposed in this Pull Request:

In order to accommodate changes that seek to remove the settings navigation bar from Calypso (Automattic/wp-calypso#53774), this PR registers the different Calypso settings pages as independent submenus of the Settings menu.

Site	Before	After
Simple
Atomic
Jetpack

Jetpack product discussion

N/A.

Does this pull request change what data or activity we track or use?

No.

Testing instructions:

Simple sites:
- Apply D62989-code to your sandbox.
- Sandbox the API and a Simple site.
- Go to https://wordpress.com/settings.
- Pick the Simple site you sandboxed above.
- Make sure that the Settings menu contains, among others, the following submenus: Performance, Writing, Discussion.
- Make sure that Settings > Writing links to Calypso or WP Admin based on the "show wp-admin pages" toggle.
- Make sure that Settings > Discussion links to Calypso or WP Admin based on the "show wp-admin pages" toggle.
Atomic sites:
- Install Jetpack Beta on an Atomic site and switch to the branch of this PR.
- Go to https://wordpress.com/settings.
- Pick the Atomic site where you've installed Jetpack Beta.
- Make sure that that Settings menu contains, among others, the following submenus: Security, Performance, Writing, Discussion, Jetpack.
- Make sure that Settings > Writing links to Calypso or WP Admin based on the "show wp-admin pages" toggle.
- Make sure that Settings > Discussion links to Calypso or WP Admin based on the "show wp-admin pages" toggle.
- Make sure that Settings > Performance is not duplicated and it links to Calypso.
Jetpack site:
- Spin up a Jetpack site running the branch of this PR and connect it to WP.com.
- Go to https://wordpress.com/settings.
- Pick the Jetpack site running the branch of this PR.
- Make sure that that Settings menu contains the following submenus: General, Security, Performance, Writing, Discussion.
- Make sure that the Jetpack submenu appears when the backups and/or security scans are configured and active.

matticbot · 2021-06-17T15:08:36Z

Caution: This PR has changes that must be merged to WordPress.com
Hello mmtr! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer and confirm D62989-code works as expected before merging this PR. Once this PR is merged, please commit the changes to WP.com. Thank you!
This revision will be updated with each commit to this PR

projects/plugins/jetpack/modules/masterbar/admin-menu/class-jetpack-admin-menu.php

github-actions · 2021-06-17T15:32:23Z

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

✅ Include a description of your PR changes.
✅ All commits were linted before commit.
✅ Add a "[Status]" label (In Progress, Needs Team Review, ...).
✅ Add testing instructions.
✅ Specify whether this PR includes any changes to data or privacy.
✅ Add changelog entries to affected projects

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 🤖

The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available.

Once your PR is ready for review, check one last time that all required checks (other than "Required review") appearing at the bottom of this PR are passing or skipped.
Then, add the "[Status] Needs Team review" label and ask someone from your team review the code.
Once you’ve done so, switch to the "[Status] Needs Review" label; someone from Jetpack Crew will then review this PR and merge it to be included in the next Jetpack release.

Jetpack plugin:

Next scheduled release: July 6, 2021.
Scheduled code freeze: June 28, 2021.

mmtr · 2021-06-18T14:17:54Z

projects/plugins/jetpack/modules/masterbar/admin-menu/class-atomic-admin-menu.php

+		// Page Optimize is active by default on all Atomic sites and registers a Settings > Performance submenu which
+		// would conflict with our own Settings > Performance that links to Calypso, so we hide it it since the Calypso
+		// performance settings already have a link to Page Optimize settings page.
+		$this->hide_submenu_page( 'options-general.php', 'page-optimize' );


the Calypso performance settings already have a link to Page Optimize settings page.

Automattic/wp-calypso#53819

BogdanUngureanu · 2021-06-22T08:40:32Z

projects/plugins/jetpack/modules/masterbar/admin-menu/class-jetpack-admin-menu.php

+		add_menu_page( esc_attr__( 'Settings', 'jetpack' ), __( 'Settings', 'jetpack' ), 'manage_options', $slug, null, 'dashicons-admin-settings', 80 );
+		add_submenu_page( $slug, esc_attr__( 'General', 'jetpack' ), __( 'General', 'jetpack' ), 'manage_options', $slug );
+		add_submenu_page( $slug, esc_attr__( 'Security', 'jetpack' ), __( 'Security', 'jetpack' ), 'manage_options', 'https://wordpress.com/settings/security/' . $this->domain );
+		add_submenu_page( $slug, esc_attr__( 'Performance', 'jetpack' ), __( 'Performance', 'jetpack' ), 'manage_options', 'https://wordpress.com/settings/performance/' . $this->domain );


Small nitpick, can we extract this menu for https://wordpress.com/settings/performance into a separate method, like we did with add_posts_menu? We can pass $slug as an argument and also reuse it in class-admin-menu.php:433

I think this is one of these cases where I prefer to sacrifice maintainability over readability and consistency. I don't like maintaining duplicated fragments of code, but it's something I'm willing to accept if the resulting code of avoiding it ends up being inconsistent with how the rest of the code behaves and/or forces the developer to jump back-and-forth into multiple source files in order have a fully understanding of the logic.

However, if folks feel strongly against this, I'm happy to refactor the code and get rid of the duplications.

I prefer to have them in just one place because at some point the arguments can change for various reasons and we might end up with a menu with some arguments on one env while having different arguments on another one, after that things can be a bit tricky to change by someone else because they might not know the context of that change.

I don't have strong feelings about it since the changes are minor, so I'm fine with both approaches :)

BogdanUngureanu · 2021-06-22T08:45:10Z

projects/plugins/jetpack/modules/masterbar/admin-menu/class-admin-menu.php

-		$this->hide_submenu_page( 'options-general.php', 'options-discussion.php' );
-		$this->hide_submenu_page( 'options-general.php', 'options-writing.php' );
+		$submenus_to_update = array(
+			'options-writing.php'    => 'https://wordpress.com/settings/writing/' . $this->domain,


Small nitpick: what if we move strings like 'https://wordpress.com/settings/writing/' . $this->domain and 'https://wordpress.com/settings/discussion/' . $this->domain into properties initialised in the constructor (e.g. $calypso_settings_writing_url)? This way we avoid duplicating the URLs in class-jetpack-admin-menu.php:230

I think this is one of these cases where I prefer to sacrifice maintainability over readability and consistency. I don't like maintaining duplicated fragments of code, but it's something I'm willing to accept if the resulting code of avoiding it ends up being inconsistent with how the rest of the code behaves and/or forces the developer to jump back-and-forth into multiple source files in order have a fully understanding of the logic.

However, if folks feel strongly against this, I'm happy to refactor the code and get rid of the duplications.

cpapazoglou · 2021-06-22T14:36:16Z

I have tested this on Simple & Atomic and works as expected, I am experiencing some env issues with Jetpack currently but I hope I can test it asap.

sdixon194 · 2021-06-22T14:46:35Z

This works for me in all environments! The only thing I noticed is some inconsistency in the menu highlighting. Clicking any of the expanded menus doesn't highlight them. Sometimes it would also retain the hover styling until I clicked away:

Is that something we'd want to address in this PR or later?

mmtr · 2021-06-22T15:13:55Z

Is that something we'd want to address in this PR or later?

That's a known issue in Calypso. It will be fixed by this PR: Automattic/wp-calypso#53824

github-actions · 2021-06-23T13:18:30Z

Great news! One last step: head over to your WordPress.com diff, D62989-code, and commit it.
Once you've done so, come back to this PR and add a comment with your changeset ID.

Thank you!

mmtr · 2021-06-23T14:25:33Z

Deployed to WP.com in r227680-wpcom.

mmtr added 2 commits June 17, 2021 16:44

Admin menu: Register Calypso settings pages as independent submenus

56ea907

Add changelog

ee5da41

mmtr added [Status] In Progress [Feature] Masterbar WordPress.com Toolbar and Dashboard customizations labels Jun 17, 2021

mmtr self-assigned this Jun 17, 2021

matticbot added the Touches WP.com Files label Jun 17, 2021

Fix positions for Hosting Configuration submenu

f0a7b3f

mmtr commented Jun 17, 2021

View reviewed changes

projects/plugins/jetpack/modules/masterbar/admin-menu/class-jetpack-admin-menu.php Show resolved Hide resolved

Fix tests

13ef007

github-actions bot added [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ Unit Tests labels Jun 17, 2021

mmtr mentioned this pull request Jun 18, 2021

Performance settings: Add link to Page Optimize settings Automattic/wp-calypso#53819

Merged

Hide Page Optimize settings

7032da4

mmtr commented Jun 18, 2021

View reviewed changes

mmtr added [Status] Needs Review To request a review from Crew. Label will be renamed soon. [Status] Needs Team Review and removed [Status] In Progress labels Jun 18, 2021

mmtr added this to the jetpack/9.9 milestone Jun 18, 2021

mmtr mentioned this pull request Jun 18, 2021

Settings: Remove horizontal navigation bar Automattic/wp-calypso#53824

Merged

samiff self-requested a review June 18, 2021 21:25

BogdanUngureanu reviewed Jun 22, 2021

View reviewed changes

sdixon194 added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. [Status] Needs Team Review labels Jun 22, 2021

mmtr added [Status] Needs Review To request a review from Crew. Label will be renamed soon. [Status] Needs Team Review and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Jun 22, 2021

sdixon194 approved these changes Jun 22, 2021

View reviewed changes

sdixon194 added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. [Status] Needs Team Review labels Jun 22, 2021

mmtr merged commit 6b32f66 into master Jun 23, 2021

mmtr deleted the update/admin-menu-calypso-settings-submenus branch June 23, 2021 13:16

github-actions bot removed the [Status] Ready to Merge Go ahead, you can push that green button! label Jun 23, 2021

mmtr mentioned this pull request Jul 6, 2021

Nav Unification: Register "Settings > Performance" submenu #20270

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Admin menu: Register Calypso settings pages as independent submenus #20100

Admin menu: Register Calypso settings pages as independent submenus #20100

mmtr commented Jun 17, 2021 •

edited

matticbot commented Jun 17, 2021

github-actions bot commented Jun 17, 2021 •

edited

mmtr Jun 18, 2021 •

edited

BogdanUngureanu Jun 22, 2021 •

edited

mmtr Jun 22, 2021

BogdanUngureanu Jun 22, 2021 •

edited

BogdanUngureanu Jun 22, 2021 •

edited

mmtr Jun 22, 2021

cpapazoglou commented Jun 22, 2021

sdixon194 commented Jun 22, 2021

mmtr commented Jun 22, 2021

github-actions bot commented Jun 23, 2021

mmtr commented Jun 23, 2021

Admin menu: Register Calypso settings pages as independent submenus #20100

Admin menu: Register Calypso settings pages as independent submenus #20100

Conversation

mmtr commented Jun 17, 2021 • edited

Changes proposed in this Pull Request:

Jetpack product discussion

Does this pull request change what data or activity we track or use?

Testing instructions:

matticbot commented Jun 17, 2021

github-actions bot commented Jun 17, 2021 • edited

mmtr Jun 18, 2021 • edited

Choose a reason for hiding this comment

BogdanUngureanu Jun 22, 2021 • edited

Choose a reason for hiding this comment

mmtr Jun 22, 2021

Choose a reason for hiding this comment

BogdanUngureanu Jun 22, 2021 • edited

Choose a reason for hiding this comment

BogdanUngureanu Jun 22, 2021 • edited

Choose a reason for hiding this comment

mmtr Jun 22, 2021

Choose a reason for hiding this comment

cpapazoglou commented Jun 22, 2021

sdixon194 commented Jun 22, 2021

mmtr commented Jun 22, 2021

github-actions bot commented Jun 23, 2021

mmtr commented Jun 23, 2021

mmtr commented Jun 17, 2021 •

edited

github-actions bot commented Jun 17, 2021 •

edited

mmtr Jun 18, 2021 •

edited

BogdanUngureanu Jun 22, 2021 •

edited

BogdanUngureanu Jun 22, 2021 •

edited

BogdanUngureanu Jun 22, 2021 •

edited