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

Use colors hook in paragraph #18148

Merged
merged 1 commit into from Dec 3, 2019
Merged

Conversation

@jorgefilipecosta
Copy link
Member

jorgefilipecosta commented Oct 28, 2019

Description

This PR uses the new useColors hook in the paragraph block. It allows us to test the hook in a more complex scenario with multiple colors. I faced some issues while using useColors on the paragraph and proposed PR #18147 as a way to address the issues.

This PR depends on #18147 and #18125.

When testing, please cherry-pick the commit from #18147 into this branch.

How has this been tested?

I verified the paragraph block looked and worked as before when selecting text and background colors. I also checked the markup and confirmed the expected classes were added.

Copy link
Contributor

epiqueras left a comment

Looking great! Thanks for picking this up 😄

packages/block-library/src/paragraph/edit.js Outdated Show resolved Hide resolved
panelChildren: ( components ) => {
return (
<ContrastChecker
{ ...{
fallbackTextColor,
fallbackBackgroundColor,
fontSize,
} }
textColor={ components.TextColor.color }
backgroundColor={ components.BackgroundColor.color }
/>
);
},
Comment on lines 143 to 155

This comment has been minimized.

Copy link
@epiqueras

epiqueras Oct 28, 2019

Contributor

Can we use the simpler contrastCheckerProps instead?

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Oct 29, 2019

Author Member

Hi @epiqueras, It seemed not to fit this use case because:

  • The default contrast mechanism is to check the contrast for all the colors, which means that the backgroundColor itself would be checked and so we would have a warning all the time.
  • The backgroundColor for contrast checking passed to hook depends on one of the colors the hook returns, which we don't have yet available when we pass it.

This comment has been minimized.

Copy link
@epiqueras

epiqueras Oct 29, 2019

Contributor

Right, you would need to call the hook twice and merge the two returned panels manually.

@jorgefilipecosta jorgefilipecosta force-pushed the update/use-colors-hook-in-paragraph branch from 3201b64 to 507f906 Oct 29, 2019
Copy link
Contributor

epiqueras left a comment

Looks good to me now.

Is the base branch intentional?

{ name: 'backgroundColor', className: 'has-background' },
],
{
panelChildren: ( components ) => {

This comment has been minimized.

Copy link
@youknowriad

youknowriad Oct 29, 2019

Contributor

I don't like this that much, makes me wonder, why this is not just an option like contrastChecker: true or something like that. (a more declarative thing)

(Not a blocker for this PR but more a thought about the API useColors)

This comment has been minimized.

Copy link
@epiqueras

epiqueras Oct 29, 2019

Contributor

#18148 (comment)

We could expand the contrast checker configuration options. cc @jorgefilipecosta

Like this:

const { TextColor, BackgroundColor, InspectorControlsColorPanel } = __experimentalUseColors(
		[
			{ name: 'textColor', property: 'color' },
			{ name: 'backgroundColor', className: 'has-background' },
		],
		{
				contrastCheckers: [{ textColor: 'white' }], // Single/multiple with literal.
				contrastCheckers: 'white', // All with literal.
				contrastCheckers: [{ textColor: ({ BackgroundColor: { value } }) => value }], // Single/multiple with dependency.
				contrastCheckers: ({ BackgroundColor: { value } }) => value // All with dependency.
				contrastCheckers: [{ textColor: true }], // Single/multiple with DOM parent background resolution.
				contrastCheckers: true, // All with DOM parent background resolution.
		}
);

What do you guys think?

This comment has been minimized.

Copy link
@youknowriad

youknowriad Oct 30, 2019

Contributor

I personally don't know enough about these features (contrast checkers) to decide if it's a great API or not but from a consumer's side, it seems reasonable and something I can learn/understand.

My question is do we need all these variations? Can we start with the simplest ones and add variations as we need them?

This comment has been minimized.

Copy link
@epiqueras

epiqueras Oct 30, 2019

Contributor

We already support // All with literal., but that doesn't help us in this PR. We should just support all 6 variations, it's simple.

This comment has been minimized.

Copy link
@epiqueras

epiqueras Nov 1, 2019

Contributor

See #18237.

@jorgefilipecosta

This comment has been minimized.

Copy link
Member Author

jorgefilipecosta commented Nov 1, 2019

Looks good to me now.

Is the base branch intentional?

Hi @epiqueras, yes the base branch was intentional. Before merging this PR we need to merge #18125 which refactors the paragraph block to be a functional component.

@jorgefilipecosta jorgefilipecosta force-pushed the update/refactor-paragraph-block-to-be-a-functional-component branch from f453813 to 5fa6e6d Nov 12, 2019
@jorgefilipecosta jorgefilipecosta changed the base branch from update/refactor-paragraph-block-to-be-a-functional-component to master Nov 25, 2019
@jorgefilipecosta jorgefilipecosta force-pushed the update/use-colors-hook-in-paragraph branch from 507f906 to e95ec7e Nov 25, 2019
@jorgefilipecosta jorgefilipecosta requested a review from ellatrix as a code owner Nov 25, 2019
@jorgefilipecosta jorgefilipecosta changed the base branch from master to add/text-color-detection-to-use-colors-hook Nov 25, 2019
@jorgefilipecosta jorgefilipecosta force-pushed the update/use-colors-hook-in-paragraph branch from e95ec7e to 5215439 Nov 25, 2019
{ InspectorControlsColorPanel }
<BackgroundColor>
<TextColor>
<ColorDetector querySelector='[contenteditable="true"]' />

This comment has been minimized.

Copy link
@youknowriad

youknowriad Nov 26, 2019

Contributor

Is it possible to have these three components just combined into one component <ApplyColors> returned by the hook?

When do we need the flexibility?

This comment has been minimized.

Copy link
@epiqueras

epiqueras Nov 26, 2019

Contributor

Whenever the targets are not direct siblings, so probably often.

This comment has been minimized.

Copy link
@youknowriad

youknowriad Nov 27, 2019

Contributor

I'm not so sure about that, in our current usage, it's always the same target. And if it's a different target, I'd probably expect useColors to be called multiple times.

This comment has been minimized.

Copy link
@epiqueras

epiqueras Nov 27, 2019

Contributor

That would create multiple panels which you would then probably have to compose. I think this is unnecessarily limiting the API to drop a few lines of code.

@jorgefilipecosta How often do you think we'll have different color application targets?

This comment has been minimized.

Copy link
@gziolo

gziolo Dec 3, 2019

Member

Are there any docs that explain how all those components created by the useColors hooks should be used?

I agree with @youknowriad that it would be much easier to consume it if there were only one component that needs to be wired without too much thinking about the tree structure and placement. Ideally, it would be only

{ InspectorControlsColorPanel }

when you assume that everything happens inside __experimentalUseColors( which already takes 3 params with different configs.

This comment has been minimized.

Copy link
@gziolo

gziolo Dec 3, 2019

Member

Anyway, if it works, we can always refine it later. It isn't different from what we have in the Heading block. We can refactor all occurrences in core blocks and discuss afterward.

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Dec 3, 2019

Author Member

Yes, I guess now that we have another usage of useColors and most of the useColors functionality was added we should look into the API format we have now and document it and see if we can increase the developer experience. cc: @epiqueras

This comment has been minimized.

Copy link
@epiqueras

epiqueras Dec 3, 2019

Contributor

InspectorControlsColorPanel just renders the panel, the components like BackgroundColor apply the colors.

I don't think the basic configuration or usage is complex, only the contrast checking configuration is, because it needs to support a lot of use cases.

Documentation will help surface any inconsistencies.

@epiqueras epiqueras force-pushed the update/use-colors-hook-in-paragraph branch from 5215439 to 8ed50a0 Nov 26, 2019
@epiqueras

This comment has been minimized.

Copy link
Contributor

epiqueras commented Nov 26, 2019

Due to the previous rebases, the changes in this PR were somehow overwriting the last commit in #18547, I fixed it with another rebase.

@epiqueras epiqueras force-pushed the update/use-colors-hook-in-paragraph branch from 8ed50a0 to 1610966 Nov 26, 2019
@jorgefilipecosta jorgefilipecosta force-pushed the update/use-colors-hook-in-paragraph branch from 1610966 to 568e8da Dec 2, 2019
@jorgefilipecosta jorgefilipecosta changed the base branch from add/text-color-detection-to-use-colors-hook to master Dec 2, 2019
@@ -143,7 +96,6 @@ function ParagraphBlock( {
<InspectorControls>
<PanelBody title={ __( 'Text Settings' ) } className="blocks-font-size">
<FontSizePicker
fallbackFontSize={ fallbackFontSize }

This comment has been minimized.

Copy link
@gziolo

gziolo Dec 3, 2019

Member

It doesn't seem to be a related change. Well, to be honest, I don't know why it was passed in the first place. You should know better :)

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Dec 3, 2019

Author Member

It was here because of historic reasons. We used a slider before and we wanted the slider position to respect the current font size the user was seeing even if the user did not explicitly set a size (was using the theme style). Right now with the current font size UI this value is not used at all and although not related to this PR, including this change here allows us to remove the with fallback styles usage.

{ InspectorControlsColorPanel }
<BackgroundColor>
<TextColor>
<ColorDetector querySelector='[contenteditable="true"]' />

This comment has been minimized.

Copy link
@gziolo

gziolo Dec 3, 2019

Member

Are there any docs that explain how all those components created by the useColors hooks should be used?

I agree with @youknowriad that it would be much easier to consume it if there were only one component that needs to be wired without too much thinking about the tree structure and placement. Ideally, it would be only

{ InspectorControlsColorPanel }

when you assume that everything happens inside __experimentalUseColors( which already takes 3 params with different configs.

@jorgefilipecosta jorgefilipecosta force-pushed the update/use-colors-hook-in-paragraph branch from 568e8da to 536ea18 Dec 3, 2019
@jorgefilipecosta jorgefilipecosta merged commit e5e2c64 into master Dec 3, 2019
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@jorgefilipecosta jorgefilipecosta deleted the update/use-colors-hook-in-paragraph branch Dec 3, 2019
@youknowriad youknowriad added this to the Gutenberg 7.1 milestone Dec 9, 2019
@ellatrix

This comment has been minimized.

Copy link
Member

ellatrix commented Jan 7, 2020

@jorgefilipecosta @epiqueras This is littering paragraph blocks with empty div elements, which will become problematic once we try to remove the block wrappers. Why can't we pass the reference of the paragraph element instead of using querying the DOM for that element?

@epiqueras

This comment has been minimized.

Copy link
Contributor

epiqueras commented Jan 7, 2020

This is littering paragraph blocks with empty div elements, which will become problematic once we try to remove the block wrappers.

How will it become problematic?

Why can't we pass the reference of the paragraph element instead of using querying the DOM for that element?

It's better to abstract away the ref handling for the consumer.

@ellatrix

This comment has been minimized.

Copy link
Member

ellatrix commented Jan 7, 2020

How will it become problematic?

The DOM will become something like

<div></div>
<p>...</p>
<div></div>
<p>...</p>
<div></div>
<p>...</p>

which might mess up CSS selectors. The goal is to eventually have the same tree as the front end.

It's better to abstract away the ref handling for the consumer.

I don't understand what you mean. Wouldn't it be better to have direct access to the ref instead of querying it?

@epiqueras

This comment has been minimized.

Copy link
Contributor

epiqueras commented Jan 7, 2020

Adding <ColorDetector /> is a simple one-liner.

The following is a lot more verbose:

const ref = useRef();
return (
	<div ref={ ref }>
		<ColorDetector nodeRef={ ref } />
	</div>
);
@epiqueras

This comment has been minimized.

Copy link
Contributor

epiqueras commented Jan 7, 2020

which might mess up CSS selectors

If a selector has trouble with this, I would argue it's not specific/good enough and should probably change.

@ellatrix

This comment has been minimized.

Copy link
Member

ellatrix commented Jan 7, 2020

It's not about our selectors, it's about theme styles.

What I would suggest is:

const ref = useRef();

return <>
	<ColorDetector nodeRef={ ref } />
	<RichText
	 	ref={ ref }
		...
	/>
</>;

That's three lines instead of one. :) The one-liner is a lot messier internally as it has to query the DOM and create empty divs for every paragraph.

@epiqueras

This comment has been minimized.

Copy link
Contributor

epiqueras commented Jan 7, 2020

I am not sure how theme styles conflict here, but I am ok with the proposed change if @jorgefilipecosta agrees.

@ellatrix

This comment has been minimized.

Copy link
Member

ellatrix commented Jan 7, 2020

@epiqueras They don't yet, but we're going to make changes to the block DOM so that theme authors need less or no editor styles. For that the DOM tree needs to be the same as the front-end (attributes are ok). It's still a long way to go, and I'm mostly looking for potential roadblocks.

@ellatrix ellatrix mentioned this pull request Jan 7, 2020
6 of 6 tasks complete
@epiqueras

This comment has been minimized.

Copy link
Contributor

epiqueras commented Jan 7, 2020

Right, but styles would have to be using things like nth-child for this to be an issue.

@ellatrix

This comment has been minimized.

Copy link
Member

ellatrix commented Jan 7, 2020

Or :first-child :) Anyway, it turns out it simplifies useColors a little as well and that it pretty much stays a one-liner.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.