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

Cover: Set custom color when applying initial background image #22564

Closed
wants to merge 34 commits into from

Conversation

ItsJonQ
Copy link

@ItsJonQ ItsJonQ commented May 22, 2020

Description

Screen Shot 2020-05-22 at 5 27 47 PM

This update adds an enhancement to the Cover block for background images. When an image is first set, it extracts and the primary color of the image and applies it as a custom background color.

This is achieved by using a library color-thief (2.6 kb gzipped), which leverages <Canvas /> for image parsing and color picking.

Here is a video demo of it working:
https://d.pr/v/f2flil

Here's the difference:

Screen Shot 2020-05-22 at 5 44 42 PM

How has this been tested?

Tested locally in Gutenberg

  • Run npm install
  • Run npm run dev
  • Add a Cover block
  • Add an image from the Media Library for the background (perhaps something colourful)
  • Check to see if the background tint matches the image
  • Check the Cover background color - make sure it's custom

Types of changes

  • This update adds a new useColorExtract hook from @wordpress/components, which handles the responsibility for image rendering/color extraction

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

Resolves comment in:
#16479 (comment)

@ItsJonQ ItsJonQ added [Type] Enhancement A suggestion for improvement. [Package] Components /packages/components [Block] Cover Affects the Cover Block - used to display content laid over a background image labels May 22, 2020
@ItsJonQ ItsJonQ requested review from youknowriad and mtias May 22, 2020 21:42
@ItsJonQ ItsJonQ self-assigned this May 22, 2020
@github-actions
Copy link

github-actions bot commented May 22, 2020

Size Change: +2.23 kB (0%)

Total Size: 1.21 MB

Filename Size Change
build/annotations/index.js 3.67 kB +3 B (0%)
build/block-directory/index.js 8.52 kB -1 B
build/block-editor/index.js 128 kB +30 B (0%)
build/block-library/index.js 139 kB +241 B (0%)
build/components/index.js 204 kB +1.97 kB (0%)
build/compose/index.js 9.67 kB +1 B
build/data-controls/index.js 1.29 kB +1 B
build/data/index.js 8.55 kB -3 B (0%)
build/deprecated/index.js 771 B -1 B
build/dom/index.js 4.47 kB -2 B (0%)
build/edit-navigation/index.js 10.7 kB +1 B
build/edit-post/index.js 305 kB +1 B
build/edit-site/index.js 19.3 kB -1 B
build/edit-widgets/index.js 12.2 kB -1 B
build/editor/index.js 45.3 kB -1 B
build/format-library/index.js 7.71 kB +2 B (0%)
build/i18n/index.js 3.56 kB +1 B
build/is-shallow-equal/index.js 710 B -1 B
build/keyboard-shortcuts/index.js 2.52 kB +1 B
build/notices/index.js 1.79 kB -1 B
build/nux/index.js 3.4 kB -2 B (0%)
build/plugins/index.js 2.56 kB -1 B
build/redux-routine/index.js 2.85 kB -4 B (0%)
build/rich-text/index.js 13.9 kB +2 B (0%)
build/shortcode/index.js 1.7 kB +1 B
build/url/index.js 4.06 kB +1 B
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/api-fetch/index.js 3.41 kB 0 B
build/autop/index.js 2.82 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/style-rtl.css 943 B 0 B
build/block-directory/style.css 942 B 0 B
build/block-editor/style-rtl.css 11 kB 0 B
build/block-editor/style.css 11 kB 0 B
build/block-library/editor-rtl.css 8.67 kB 0 B
build/block-library/editor.css 8.67 kB 0 B
build/block-library/style-rtl.css 7.59 kB 0 B
build/block-library/style.css 7.59 kB 0 B
build/block-library/theme-rtl.css 741 B 0 B
build/block-library/theme.css 741 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 47.8 kB 0 B
build/components/style-rtl.css 15.5 kB 0 B
build/components/style.css 15.4 kB 0 B
build/core-data/index.js 12.2 kB 0 B
build/date/index.js 31.9 kB 0 B
build/dom-ready/index.js 568 B 0 B
build/edit-navigation/style-rtl.css 868 B 0 B
build/edit-navigation/style.css 871 B 0 B
build/edit-post/style-rtl.css 6.24 kB 0 B
build/edit-post/style.css 6.22 kB 0 B
build/edit-site/style-rtl.css 3.13 kB 0 B
build/edit-site/style.css 3.13 kB 0 B
build/edit-widgets/style-rtl.css 2.55 kB 0 B
build/edit-widgets/style.css 2.55 kB 0 B
build/editor/editor-styles-rtl.css 492 B 0 B
build/editor/editor-styles.css 493 B 0 B
build/editor/style-rtl.css 3.8 kB 0 B
build/editor/style.css 3.8 kB 0 B
build/element/index.js 4.65 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/style-rtl.css 547 B 0 B
build/format-library/style.css 548 B 0 B
build/hooks/index.js 2.13 kB 0 B
build/html-entities/index.js 621 B 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.12 kB 0 B
build/list-reusable-blocks/style-rtl.css 476 B 0 B
build/list-reusable-blocks/style.css 476 B 0 B
build/media-utils/index.js 5.31 kB 0 B
build/nux/style-rtl.css 671 B 0 B
build/nux/style.css 668 B 0 B
build/primitives/index.js 1.41 kB 0 B
build/priority-queue/index.js 789 B 0 B
build/server-side-render/index.js 2.77 kB 0 B
build/token-list/index.js 1.27 kB 0 B
build/viewport/index.js 1.85 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

@ItsJonQ
Copy link
Author

ItsJonQ commented May 22, 2020

I'm noticing a bug where we need to update the color when changing/updating the background image.

Will need to work on that logic next!

@ItsJonQ
Copy link
Author

ItsJonQ commented May 23, 2020

Aww. It looks like color-thief has some incompatible licenses. I believe it's because the library attempts to accommodate both web and node.js. It looks like the node.js dependencies are making the license checker unhappy

@strarsis
Copy link
Contributor

@ItsJonQ
Copy link
Author

ItsJonQ commented May 25, 2020

Heya @strarsis ,

Thanks for the suggestions! I checked out the libraries. We wouldn't be able to use those as those are node.js based. We need a web-based solution :).

Thankfully, I was able to adjust the module to fit our requirements

Update: I'm unsure about gradients for now 😊

@mtias
Copy link
Member

mtias commented May 26, 2020

Reminds me of https://github.com/mtias/tonesque :)

@ItsJonQ
Copy link
Author

ItsJonQ commented May 26, 2020

@mtias Oh man. I recognize some of those RGB colour formulas 😂 (I worked on lower colour interpreting/parsing stuff in JS a couple of years ago)

@mtias
Copy link
Member

mtias commented May 26, 2020

I like this exploration. I think it might also make sense to add a button / action to set it to "dominant color" if you have picked another one. cc @jasmussen

@ItsJonQ
Copy link
Author

ItsJonQ commented May 26, 2020

I think it might also make sense to add a button / action to set it to "dominant color" if you have picked another one

@mtias OoOOooo! Interesting to see what @jasmussen has for design ideas!

Note: The color extractor can actually grab a palette of colours! Not saying we have to use that feature. Just pointing that out, in case it sparks any ideas 😉

@jasmussen
Copy link
Contributor

For whatever reason, I couldn't get this PR to work for me, no color is stolen by the thief:

before

That said, and judging by your video, this is really really cool and I share Matías' excitement.

Very first question: can we add text color to the cover block, and set that to dark or light depending on the luminosity of the background color eye-dropped? You'd still be able to override colors on a per-paragraph basis, or even unset the text color on the cover block. But it would help create a nice contrasty basic experience.

Transversal, but related: this tool could theoretically help the block UI. We currently know whether a theme has a light or dark background because the theme can tell us, however even then we have cases where the background color is sub-optimal for the block UI. Like this blue group with a light-gray spacer inside:

Screenshot 2020-05-27 at 10 47 51

Back to the PR in question, this is so lovely, it makes the end result look much better. It reminds me of the "faded" filtered images that are popular on Instagram, but more than that it's just a better overlay with the text as a background.

I'll put together a small design tool iteration for the cover block sidebar that includes a "pick dominant color" button — for the time being though, feel free to just go ahead and make that button if you can, because I agree with Matías, it'd be nice to be able to pick this after the fact too. I might want to go back to an old cover block I had and set this, or I might explore other colors and then want to go back to the auto-picked one.

@jasmussen
Copy link
Contributor

Here's a quick experiment:

Frame 146

As with many of these G2 designs, they rely on some designs landing in the sidebar that aren't there yet. Please consider this like a destination we'll work towards.

@ItsJonQ ItsJonQ added the [Status] In Progress Tracking issues with work in progress label May 28, 2020
@ItsJonQ
Copy link
Author

ItsJonQ commented May 28, 2020

@jasmussen Thanks for the feedback!

...no color is stolen by the thief:

Oh darn! Do you think you can forward me a link to that image? Probably worth test it (on my end) using the same one that's giving you issues.

Very first question: can we add text color to the cover block, and set that to dark or light depending on the luminosity of the background color eye-dropped?

You bet! I think that would be fun! Probably a little tricky... but worth trying!

...it'd be nice to be able to pick this after the fact too. I might want to go back to an old cover block I had and set this, or I might explore other colors and then want to go back to the auto-picked one.

I'll give this a shot too! Sounds exciting :D

@jasmussen
Copy link
Contributor

Very curious, for a moment I thought it was a missing npm install I did, but that didn't help. I still think it's possibly something on my end, but I still can't get it to work. The pictures I keep using are these: https://cloudup.com/cAYtklfVZtk

You bet! I think that would be fun! Probably a little tricky... but worth trying!

Not necessary for this PR at all, just an idea!

@jasmussen
Copy link
Contributor

I see this:

cover

It feels good. I think we can still think about some indication that the added color is magically added, like a dot or something. But I think there's such value in this that I'd be fine with merging as is and improving design tools separately.

Copy link
Contributor

@mcsf mcsf left a comment

Choose a reason for hiding this comment

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

I'm happy to see those passing e2e tests following cec9121 :)

@@ -110,11 +111,13 @@ export const PanelColorGradientSettingsInner = ( {
return null;
}

const mergedColors = [ ...customColors, ...colors ];
Copy link
Contributor

Choose a reason for hiding this comment

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

Depending on how often PanelColorGradientSettingsInner re-renders as a user interacts with the block, might be good to:

Suggested change
const mergedColors = [ ...customColors, ...colors ];
const mergedColors = useMemo( () => [ ...customColors, ...colors ], [
customColors,
colors,
] );
;

Naturally, this supposes that references for customColors and colors are preserved when possible.

};

ColorThief.prototype.getImageData = function ( imageUrl, callback ) {
const xhr = new XMLHttpRequest();
Copy link
Contributor

Choose a reason for hiding this comment

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

Tapping into getMedia could be good.

Comment on lines +597 to +598
name: __( 'Image dominant color' ),
slug: __( 'image-dominant-color' ),
Copy link
Contributor

Choose a reason for hiding this comment

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

Some context or translators: notes could be good for these.

packages/components/src/index.js Outdated Show resolved Hide resolved
@@ -1,6 +1,7 @@
/**
* External dependencies
*/
import { noop } from 'lodash';
import classnames from 'classnames';
import FastAverageColor from 'fast-average-color';
Copy link
Member

Choose a reason for hiding this comment

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

Hi @ItsJonQ, the cover block was already computing the color of an image using the FastAverageColor module. I think we should avoid using two different modules for the same task so I guess we should opt for one of the modules and update the code to use the chosen module.

Copy link
Author

Choose a reason for hiding this comment

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

@jorgefilipecosta Oh! I wasn't aware of that. Thank you for pointing that out. I'll make an update

Base automatically changed from master to trunk March 1, 2021 15:43
@annezazu
Copy link
Contributor

Closing this out due to inactivity :)

@annezazu annezazu closed this Jul 27, 2022
@mtias
Copy link
Member

mtias commented Sep 10, 2022

@annezazu I'd still love to try something like this. Can we transfer it to an issue?

@annezazu
Copy link
Contributor

Done :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Cover Affects the Cover Block - used to display content laid over a background image [Package] Components /packages/components [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants