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 ectoplasm color scheme (Method A) #47122

Merged
merged 21 commits into from
Nov 9, 2020
Merged

Conversation

mreishus
Copy link
Contributor

@mreishus mreishus commented Nov 4, 2020

One part of #47036
Method A is defined in #47103. There's a "Method B" copy of the PR here: #47075.

Changes proposed in this Pull Request

  • Add ectoplasm color scheme

Testing instructions

  • Visit /me/account
  • Choose ectoplasm and save

To see the wp-admin color scheme, create an atomic site, then visit /wp-admin/profile.php, choose color scheme and save.

Calypso:
2020-11-04_15-54
Wp-Admin:
2020-11-04_15-54_1

Known Issues

  • Big: All lightness values of both primary and accent colors, -10 through -90, are set to the same color. How do we generate these while maintaining a seamless experience between wp-admin and calypso? Here's how color-studio generates them.
  • Primary and accent colors are the same. Does this cause an issue?

@matticbot
Copy link
Contributor

@mreishus mreishus added the [Feature] Calypso & wp-admin Navigation All navigation in Calypso and wp-admin, and the unified transitions between the two. label Nov 4, 2020
@mreishus mreishus self-assigned this Nov 4, 2020
@matticbot
Copy link
Contributor

matticbot commented Nov 5, 2020

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

App Entrypoints (~8 bytes added 📈 [gzipped])

name        parsed_size           gzip_size
entry-main        +12 B  (+0.0%)       +8 B  (+0.0%)

Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used.

Sections (~62 bytes added 📈 [gzipped])

name     parsed_size           gzip_size
account       +205 B  (+0.1%)      +62 B  (+0.1%)

Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to.

Async-loaded Components (~136 bytes added 📈 [gzipped])

name                                           parsed_size           gzip_size
async-load-design-blocks                            +205 B  (+0.0%)      +55 B  (+0.0%)
async-load-design                                   +205 B  (+0.0%)      +67 B  (+0.0%)
async-load-lib-preferences-helper                    +12 B  (+0.1%)       +7 B  (+0.2%)
async-load-calypso-blocks-inline-help-popover        +12 B  (+0.0%)       +7 B  (+0.0%)

React components that are loaded lazily, when a certain part of UI is displayed for the first time.

Legend

What is parsed and gzip size?

Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

@mreishus mreishus force-pushed the add/color-scheme-ectoplasm-alt branch from 908cce4 to d748b64 Compare November 5, 2020 18:53
@mreishus
Copy link
Contributor Author

mreishus commented Nov 5, 2020

Listing issues that arise from setting all primary-10 - primary-90 and accent-10 - accent-90 colors to wp-admin highlight color. (Starting with one and will edit others in)

SectionNav Hover - Unreadable text
2020-11-05_13-16
2020-11-05_13-16_1
Hover background is color-primary-0

Busy bar isn't animated
2020-11-05_13-20
2020-11-05_13-20_1
color-accent and color-accent-60

Credit Card is unreadable
2020-11-05_13-22
2020-11-05_13-22_1
Background is color-primary-0, border is color-primary-5 and color-primary-10

Line Chart series blend together
2020-11-05_13-30
2020-11-05_13-30_1
color-primary-dark, color-accent, color-primary-light

PieChart
2020-11-05_13-31
2020-11-05_13-32
color-primary-dark, color-accent, color-primary-light

ProductCard doesn't show selection as clearly
2020-11-05_13-32_1
2020-11-05_13-33
--color-primary-rgb - this is a bug because my -rgb variants include rgb() when they shouldn't

ProgressBar doesn't animate
2020-11-05_13-35
2020-11-05_13-35_1
color-primary, color-primary-light

Some text uses color-primary-dark and is hard to read.
2020-11-05_14-08

@mreishus
Copy link
Contributor Author

mreishus commented Nov 5, 2020

So, having all -10 -90 assigned to the same color was causing some issues as illustrated above. I used color-studio to generate a range of colors and it fixes the issue.

Since we set the sidebar colors exactly, we can make sure that those match wp-admin exactly and don't use the "palette" colors. So this way, we retain parity with wp-admin but provide calypso with some of the extra colors it wants.

in data/color-definitions.js

    {
      name: 'Ectoplasm Green',
      default: 50,
      specs: {
        hue_start: 70,
        hue_end: 72,
        hue_curve: 'easeOutSine',
        sat_steps: [
          8, 27, 56, 80, 100, 100, 100, 100, 100, 100, 100, 100
        ],
        lum_steps: [
          96, 96, 95, 94, 87, 75.1, 61.6, 49, 40.5, 31, 20, 11
        ]
      }
    }

2020-11-05_15-01

2020-11-05_14-56
2020-11-05_14-57
2020-11-05_14-57_1
2020-11-05_14-58
2020-11-05_14-58_1
2020-11-05_14-58_2
2020-11-05_14-58_3
2020-11-05_14-59

@mreishus mreishus marked this pull request as ready for review November 5, 2020 21:03
@mreishus mreishus requested review from sixhours, lcollette and a team November 5, 2020 21:04
@matticbot matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Nov 5, 2020
Copy link
Contributor

@sixhours sixhours left a comment

Choose a reason for hiding this comment

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

Took this for a quick spin! I noticed the outline around the help button could be tweaked to use a variation of the green:

Screen Shot 2020-11-06 at 9 03 06 AM

What do you think about adding a darker purple for the top bar border color to give it a little separation from the side nav?

Screen Shot 2020-11-06 at 9 00 57 AM

Screen Shot 2020-11-06 at 9 21 01 AM

Copy link
Contributor

@cpapazoglou cpapazoglou left a comment

Choose a reason for hiding this comment

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

Thanks @mreishus for wrangling this!
Generally it works as expected. I navigated around and checked the components page! Let's check if we can easily address the following:

Menu Heading should be the same as links or at least vivid enough

@cpapazoglou
Copy link
Contributor

Took this for a quick spin! I noticed the outline around the help button could be tweaked to use a variation of the green:

Screen Shot 2020-11-06 at 9 03 06 AM

What do you think about adding a darker purple for the top bar border color to give it a little separation from the side nav?

Screen Shot 2020-11-06 at 9 00 57 AM Screen Shot 2020-11-06 at 9 21 01 AM

@sixhours thanks for chiming in! As it seems all wp-admin color schemes make no distinction between the masterbar and the sidebar in terms of colour or bordering. I agree it would be best, but I am not sure that we should start making changes to already wp-admin defined colour schemes. This would increase maintainability of schemes and make the scope of porting the colour schemes even larger. @davemart-in , @lcollette what is your take on this?

@sixhours
Copy link
Contributor

sixhours commented Nov 6, 2020

The /wp-admin version of Ectoplasm shows a border for me:

Screen Shot 2020-11-06 at 10 36 29 AM

@cpapazoglou
Copy link
Contributor

The /wp-admin version of Ectoplasm shows a border for me:

Screen Shot 2020-11-06 at 10 36 29 AM

Interesting, the core version doesn't have one... It is a wpcom hack.

That said, I am not sure how we should proceed... wpcom-admin-bar.css contains a dozen of overrides.

@mreishus
Copy link
Contributor Author

mreishus commented Nov 6, 2020

Variable scope change

Done.

Notification circle color in masterbar:

Done.

Masterbar bottom border color:

As noted above, it seems like this looks different depending on if you're on atomic or not. I'm okay adding a bottom border since there is already support in it in calypso, and I only have to change one line. I've made the change here. I guess it ultimately depends on which version of WP we want to be consistent with.

FAB Outer ring color

It is already based on the green highlight color, but can be hard to tell:
2020-11-06_11-21
It doesn't seem too out of line with an existing theme in calypso, like sunset:
2020-11-06_11-23
This one I'm hesitant to change because I'd have to make --color-primary-dark lighter, which might affect readability of some text (like the CreditCard code listing change a few posts up)

Sidebar top level menu text brightness

I can do this, (And I have), but I'm wondering if it's the right thing to do.

In calypso, it's normal for the top level menus to have less contrast than the inner menus. Examples:
2020-11-06_10-59
2020-11-06_11-00

In wp-admin, I see the outer menus brighter
2020-11-06_11-15

So we have three options for the ports
A (Dimmer outer, conforms to calypso feeling)
2020-11-06_11-14-bright-inner

B (Dimmer inner, conforms to wp-admin feeling)
2020-11-06_11-12-bright-outer

C (Both bright, compromise)
2020-11-06_11-11-same-bright

What should we go with? I have "C" pushed right now.

@mreishus mreishus mentioned this pull request Nov 6, 2020
94 tasks
@mreishus
Copy link
Contributor Author

mreishus commented Nov 6, 2020

🤦‍♂️ I just realized that these don't work when the nav unification flag is on, because these overrides take precedence. I made a crude fix that re-specifies all the theme variables inside a .is-nav-unification { } selector, but will think about a better fix on Monday.

@cpapazoglou
Copy link
Contributor

🤦‍♂️ I just realized that these don't work when the nav unification flag is on, because these overrides take precedence. I made a crude fix that re-specifies all the theme variables inside a .is-nav-unification { } selector, but will think about a better fix on Monday.

Changes in sidebar-unified/style.scss will ultimately reside in the default theme _default.scss and they will be overridden by each colour-scheme. Till then, I pushed a commit, making your idea DRY. It should be enough.

Copy link
Contributor

@cpapazoglou cpapazoglou left a comment

Choose a reason for hiding this comment

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

Thanks for pushing the changes @mreishus!

LGTM!

@mreishus mreishus merged commit 232ea97 into master Nov 9, 2020
@mreishus mreishus deleted the add/color-scheme-ectoplasm-alt branch November 9, 2020 22:03
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Nov 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Calypso & wp-admin Navigation All navigation in Calypso and wp-admin, and the unified transitions between the two.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants