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

Add Unified Admin Color Schemes module skeleton #17750

Closed
wants to merge 1 commit into from

Conversation

getdave
Copy link
Contributor

@getdave getdave commented Nov 9, 2020

Adds a new module skeleton to provide a starting point for all code required for the admin color scheme unification on Dotcom.

Changes proposed in this Pull Request:

  • Adds new module unified-admin-color-schemes.

Jetpack product discussion

Automattic/wp-calypso#45675

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

Nope

Testing instructions:

  • Checkout PR.
  • Add some debugging to the uacs_setup_admin function.
  • Check the debugging is run.

Proposed changelog entry for your changes:

  • Adds unified-admin-color-schemes module.

@getdave getdave requested a review from a team November 9, 2020 11:12
@getdave getdave self-assigned this Nov 9, 2020
@jetpackbot
Copy link

Fails
🚫 Please add these new PHP files to PHPCS required list in bin/phpcs-requirelist.js for automatic linting: modules/unified-admin-color-schemes.php

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

This is an automated check which relies on PULL_REQUEST_TEMPLATE. We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

Generated by 🚫 dangerJS against fba352d

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.

This works okay as described. Let's add the file to the list as the bot comment above suggests.

The place for the module looks good to me but for this case, I'd rather have a secondary check from somebody in the crew, just to make sure we land it appropriately.

My other note is about the uacs shorthand. These always seem cryptic to me and I'm not sure whether that's a built-in system name or something we came up with. Would it make sense to write those words out or do something like jetpack_color_schemes instead? I'll leave this up to you, since I don't think it breaks any rule.

@marekhrabe marekhrabe added the [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. label Nov 9, 2020
@marekhrabe marekhrabe requested review from jeherve and a team November 9, 2020 13:03
@jeherve jeherve added General [Feature] Masterbar WordPress.com Toolbar and Dashboard customizations [Type] Feature Request [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 fellow Jetpack developers. Label will be renamed soon. [Status] Needs Team Review labels Nov 9, 2020
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

The place for the module looks good to me but for this case, I'd rather have a secondary check from somebody in the crew, just to make sure we land it appropriately.

Discussion: p1604926678074100-slack-CJS75TX3R

My personal preference would be for us to expand the existing masterbar module, which already gets preferential treatment on Atomic sites, to include all those UI tweaks we'll be making for Atomic sites.

As part of the changes, we could even rename the module; as long as the module slug (the entry point's file name, modules/masterbar.php, remains the same, we won't lose anything.

My other note is about the uacs shorthand. These always seem cryptic to me and I'm not sure whether that's a built-in system name or something we came up with.

I think it'd be okay, and more readable to switch to a namespace that better describes this?

@obenland
Copy link
Member

obenland commented Nov 9, 2020

See #17629 for a first attempt at adding to the masterbar module

@getdave
Copy link
Contributor Author

getdave commented Nov 10, 2020

Note that if we proceed with this approach we'll want to structure it as a class similar to how we'd done in #17762. We could also consider namespacing.

@obenland
Copy link
Member

Fixed in #17762

@obenland obenland closed this Nov 16, 2020
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 General [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! [Type] Feature Request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants