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

Modify masterbar module to include new unified admin color schemes file #17762

Merged
merged 10 commits into from
Nov 11, 2020

Conversation

getdave
Copy link
Contributor

@getdave getdave commented Nov 10, 2020

Alternative to #17750.

As discussed in p1604929101083300-slack-ajax this PR repurposes and extends the existing Masterbar module to become a more all purpose module for any customizations required to the WPAdmin Dashboard experience on WP.com.

Specifically, where there was previously only the A8C_WPCOM_Masterbar class, we now add an additional A8C_WPCOM_Unified_Admin_Color_Schemes class and init that from the main file as well.

In the future, other classes could be added.

Please note that @obenland has already started work on customising the masterbar module here so we could look to land that. However, as it stands my PR (ie: this one) contains less code and is more "bare bones" and thus might be easier to merge.

Changes proposed in this Pull Request:

  • Add A8C_WPCOM_Unified_Admin_Color_Schemes class to masterbar module and init from main file.
  • Repurpose master bar module to become an all-purpose module for any customizations required to the WPAdmin Dashboard experience on WP.com.
  • Add PHP namespace to the module.
  • Move individual concerns into separate directories (eg: /masterbar, /unified-admin-color-schemes...etc).

Jetpack product discussion

  • paYJgx-17t-p2#comment-1391
  • p1604929101083300-slack-ajax

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

No

Testing instructions:

  • Checkout PR
  • Modify init method of modules/masterbar/unified-admin-color-schemes.php to include some debug output.
  • Check that debug output is shown only on admin pages.
  • Verify existing masterbar works as previously.

Proposed changelog entry for your changes:

  • Repurpose master bar module to become an all-purpose module for any customizations required to the WPAdmin Dashboard experience on WP.com.
  • Add unified color schemes class to masterbar module and init from main file.
  • Add PHP namespace to the module.
  • Move individual concerns into separate directories (eg: /masterbar, /unified-admin-color-schemes...etc).

@jetpackbot
Copy link

jetpackbot commented Nov 10, 2020

Scheduled Jetpack release: December 1, 2020.
Scheduled code freeze: November 23, 2020

E2E results is available here (for debugging purposes): https://jetpack-e2e-dashboard.herokuapp.com/pr-17762

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Generated by 🚫 dangerJS against 6a4090d

@getdave
Copy link
Contributor Author

getdave commented Nov 10, 2020

I've just noticed that @obenland has already started work on customising the masterbar module here so we could look to land that. However, as it stands my PR (ie: this one) contains less code and is more "bare bones" and thus might be easier to merge.

@jeherve jeherve added the [Feature] Masterbar WordPress.com Toolbar and Dashboard customizations label Nov 10, 2020
@jeherve jeherve added [Pri] Normal [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it labels Nov 10, 2020
@getdave getdave added the [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. label Nov 10, 2020
@marekhrabe
Copy link
Contributor

This works well for me and the testing echo I've done in the color scheme class has worked in wp-admin correctly ✅

I think we are missing one last step and that is updating the module definition in modules/module-headings.php, since the name and description were updated. yarn build should get you sorted and you should commit the updated file.

Also fixes to align with new PHPCS lint rules
marekhrabe
marekhrabe previously approved these changes Nov 10, 2020
@marekhrabe marekhrabe removed the [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. label Nov 10, 2020
@getdave getdave added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] Needs Team Review labels Nov 10, 2020
@getdave getdave requested a review from jeherve November 10, 2020 13:33
@obenland obenland added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Nov 10, 2020
Copy link
Contributor

@marekhrabe marekhrabe left a comment

Choose a reason for hiding this comment

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

I've rested everything again with a code confirmed to be run in the color schemes class. With remaining issues externalized, I think we should be good proceeding and have a base for other color scheme tasks.

@marekhrabe marekhrabe added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Nov 11, 2020
@obenland obenland merged commit 020b7b5 into master Nov 11, 2020
@obenland obenland deleted the try/repurpose-masterbar-module branch November 11, 2020 17:28
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Nov 11, 2020
@getdave
Copy link
Contributor Author

getdave commented Nov 12, 2020

FYI anyone who comes to this and wonders what happened to syncing this code via Fusion please see

p1605115584001400-slack-ajax-reports

we can sync the code later if we need to (it's currently not synced by fusion)

@getdave
Copy link
Contributor Author

getdave commented Nov 12, 2020

Thanks for merging @obenland. I really appreciate that.

anomiex added a commit that referenced this pull request Nov 23, 2020
The file was edited to pass phpcs in #17762, so now every `yarn build`
overwrites it with a changed version. Attempting to commit that changed
version will wind up whining about all the phpcs failures in the
generated version, and fixing them (mostly with phpcbf, some manually)
will wind up as a no-op.

This patch updates tools/build-module-headings-translations.php to fix
some of the errors and emit `phpcs:disable` for two others that isn't
worth trying to generate the way the WordPress coding standards want.
kraftbj pushed a commit that referenced this pull request Nov 24, 2020
The file was edited to pass phpcs in #17762, so now every `yarn build`
overwrites it with a changed version. Attempting to commit that changed
version will wind up whining about all the phpcs failures in the
generated version, and fixing them (mostly with phpcbf, some manually)
will wind up as a no-op.

This patch updates tools/build-module-headings-translations.php to fix
some of the errors and emit `phpcs:disable` for two others that isn't
worth trying to generate the way the WordPress coding standards want.
jeherve added a commit that referenced this pull request Dec 1, 2020
In #17762 and #17783 we've refactored the Masterbar, but forgot to update references to it in the Calypsoify feature, thus causing Fatal errors.
jeherve added a commit that referenced this pull request Dec 3, 2020
In #17762 and #17783 we've refactored the Masterbar, but forgot to update references to it in the Calypsoify feature, thus causing Fatal errors.
jeherve added a commit that referenced this pull request Dec 7, 2020
Fixes #17989

In #17762 and #17783 we've refactored the Masterbar, but forgot to update the reference to the script that got moved along with this.
jeherve added a commit that referenced this pull request Dec 8, 2020
Fixes #17989

In #17762 and #17783 we've refactored the Masterbar, but forgot to update the reference to the script that got moved along with this.
jeherve added a commit that referenced this pull request Dec 8, 2020
Fixes #17989

In #17762 and #17783 we've refactored the Masterbar, but forgot to update the reference to the script that got moved along with this.
jeherve added a commit that referenced this pull request Dec 8, 2020
Fixes #17989

In #17762 and #17783 we've refactored the Masterbar, but forgot to update the reference to the script that got moved along with this.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Masterbar WordPress.com Toolbar and Dashboard customizations [Pri] Normal [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants