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

Admin Color Schemes: generate wp-admin color schemes from Calypso counterparts #17828

Merged
merged 34 commits into from
Dec 14, 2020

Conversation

frontdevde
Copy link
Contributor

@frontdevde frontdevde commented Nov 17, 2020

Changes proposed in this Pull Request

This PR adds 9 new wp-admin color schemes based on existing Calypso color schemes.

In Jetpack these will be available to select in the wp-admin user profile when the WordPress.com toolbar option in the Writing settings section is active.

Context for this addition is that for Simple and WoA sites we need to have all Calypso color schemes available in wp-admin for the Nav unification project. For details please see Automattic/wp-calypso#45675

All color scheme options with this change:

Screenshot 2020-11-26 at 11 15 15

Testing instructions

For Jetpack:

  • In the Jetpack settings ensure that in the Writing section the WordPress.com toolbar option is active
  • Navigate to your user profile in /wp-admin/profile.php
  • Check if all Calypso color schemes are present
  • Compare new color schemes to their Calypso counterparts

For WoA:

  • Use Jetpack Beta and activate try/admin-color-schemes branch

For Simple:
Tricky because we hide the wp-admin profile settings. Needs another Diff applied.

  • Apply D52880 (diff for this PR) to your sandbox
  • Apply D52376 (does the sync) to your sandbox
  • Change color scheme settings in Calypso /me/account to Sakura
  • Go to wp-admin and reload. It should display in Sakura colors
    Edit: color don't display anymore as the build moved from TC to Compass on svn ci (see slack-C4GAP2RHD/p1607525905317500). Double check if the path for the color scheme is as expected for simple.

Todos / development task list

Known tasks:

  • Add admin theme file override for main nav background color to be different from sidebar

  • Add admin theme file override for current menu item icon color on hover (shouldn't change)

  • Add admin theme file override for selected submenu menu item color on hover (shouldn't change)

  • Use rtl css if site is RTL Remove RTL from build

  • List of color schemes to port over:

    • Aquatic (i.e. former Calypso Ocean) - Scaffolding
    • Aquatic (i.e. former Calypso Ocean) - Refined
    • Classic Blue - Scaffolding
    • Classic Blue - Refined
    • Classic Bright - Scaffolding
    • Classic Bright - Refined
    • Classic Dark - Scaffolding
    • Classic Dark - Refined
    • Contrast - Scaffolding
    • Contrast - Refined
    • Nightfall - Scaffolding
    • Nightfall - Refined
    • Powder Snow - Scaffolding
    • Powder Snow - Refined
    • Sakura - Scaffolding
    • Sakura - Refined
    • Sunset - Scaffolding
    • Sunset - Refined

@frontdevde frontdevde added [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Feature] Masterbar WordPress.com Toolbar and Dashboard customizations labels Nov 17, 2020
@frontdevde frontdevde self-assigned this Nov 17, 2020
@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello frontdevde! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer and confirm D52880-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

@jetpackbot
Copy link

jetpackbot commented Nov 17, 2020

Warnings
⚠️ The Privacy section is missing for this PR. Please specify whether this PR includes any changes to data or privacy.
⚠️ "Proposed changelog entry" is missing for this PR. Please include any meaningful changes

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 9272275

@jeherve jeherve added this to the 9.3 milestone Nov 23, 2020
@frontdevde frontdevde force-pushed the try/admin-color-schemes branch from c866cf2 to ccba2e2 Compare November 24, 2020 09:40
@frontdevde
Copy link
Contributor Author

frontdevde commented Nov 26, 2020

Handover notes prior to AFK:

  • Calypso color schemes are all implemented. So far mainly tested with a Jetpack site, needs further testing within Nav Unification context.

  • The build is generating RTL stylesheets but they are not being used yet. Instead of just loading the default file in all cases plugins_url( 'colors/classic-blue/colors.css', __FILE__ ) we might want to check for is_rtl() and load the RTL file instead. That said, looking at the styles in _admin.scss I'm not certain it's actually necessary.

  • The Jetpack team expressed interest in having the admin color schemes available as a feature for all users, not just Simple and WoA. The issue I'm seeing is that right now the admin color schemes are only showing once the WordPress.com toolbar has been enabled. The location made sense for what we're trying to do with Nav Unification, I'm not sure that's still the case if Jetpack would like to have this as a feature for all their users.

cc @cpapazoglou @getdave

@obenland
Copy link
Member

The Jetpack team expressed interest in having the admin color schemes available as a feature for all users

@jeherve Where would be a good place for this to live? I assume you'd just want this to work out of the box?

@cpapazoglou cpapazoglou self-assigned this Dec 1, 2020
@jeherve
Copy link
Member

jeherve commented Dec 1, 2020

Where would be a good place for this to live?

I think it would be fine as part of the masterbar, with the rest of the changes you've been making. If folks are interested in the colors, they can enable the module.

@cpapazoglou
Copy link
Contributor

cpapazoglou commented Dec 1, 2020

  • I agree that we shouldn't create rtl .css files.
  • Regarding the place for the code, I am not sure that masterbar module is the correct place. It doesn't seem to be currently copied / enabled to wpcom where we also need color-schemes. @obenland I guess the same applies to Masterbar: Add menu structure #17629 also. Do you have a plan in mind or am I missing something?

@jeherve
Copy link
Member

jeherve commented Dec 2, 2020

I am not sure that masterbar module is the correct place. It doesn't seem to be currently copied / enabled to wpcom where we also need color-schemes.

Noting that files don't need to be in the same location in both environments. Individual files can be kept in sync, but required differently. In Jetpack, we require the different parts of the Masterbar here, that file only getting loaded when the Masterbar module is active:

require __DIR__ . '/masterbar/masterbar/class-masterbar.php';
require __DIR__ . '/masterbar/admin-color-schemes/class-admin-color-schemes.php';

On WordPress.com, we could choose to have a different loader file, pointing to the same file.

@obenland
Copy link
Member

obenland commented Dec 2, 2020

@jeherve @cpapazoglou I reclaimed D53264-code to sync the admin-color-scheme class to dotcom. Once that's accepted and merged, the changes in this PR can be synced.

@cpapazoglou
Copy link
Contributor

@jeherve we have used a .scss building to .css process here https://github.com/Automattic/jetpack/pull/17828/files#diff-3779894f3734e15106366de177129f7cee7e6745e3ea14dfd9f0d0d205612ba6

I am afraid that we have to use plain .css so that this files can be synced to wpcom? Or can we just remove the gitignore rule https://github.com/Automattic/jetpack/pull/17828/files#diff-bc37d034bad564583790a46f19d807abfe519c5671395fd494d8cce506c42947 and make sure we commit the builded .css files?

@obenland obenland force-pushed the try/admin-color-schemes branch from 2e2a61a to 201688e Compare December 4, 2020 20:21
@obenland obenland marked this pull request as ready for review December 4, 2020 20:21
@frontdevde frontdevde force-pushed the try/admin-color-schemes branch from 81f9843 to 4f81208 Compare December 10, 2020 12:29
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 took a fair bit of effort but I've managed to use it on all platforms without an issue.

Jetpack on my local site ✅
Jetpack install via Jetpack beta ✅
Simple ✅
Atomic ✅

@frontdevde frontdevde added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] Needs Team Review labels Dec 10, 2020
@jeherve jeherve 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 Dec 10, 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.

This tests well for me. 🚢

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 Touches WP.com Files [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