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

Try/group block custom text color #19181

Merged
merged 6 commits into from Jan 20, 2020
Merged

Conversation

@glendaviesnz
Copy link
Contributor

glendaviesnz commented Dec 16, 2019

Description

With the current group block, if a custom background colour is selected that does not contrast well with the text in child blocks there is no way to rectify this other than applying a suitable text colour to every child block within the group

This PR adds a text colour selector to the Group Block inspector controls in addition to the background colour selector. It also refactors the block to use the new useColors hook instead of withColours

The text colour gets applied to the children by the default css cascade, so applying a text colour to to a child block, at any level of nesting, will override the colour set on the parent group block, and moving the child block out of the parent will also remove the colour.

Possible additions for follow on PRs, based on feedback to-date:

  • Indicate in some way in the child component that a colour is being inherited from the parent
  • Automatically select a contrasting text colour if a custom background colour is selected - but it would probably make more sense to add this as an option in the useColors hook than in this specific component

How has this been tested?

Tested manually by:

Adding children to a group block at multiple levels of nesting and making sure that text colour apply to group block is applied to text of children, and that any colour applied to child blocks overrides text colour from parent group block
Adding a group block from existing master branch and then opening/editing/saving to make sure there are no backwards compatibility issues.

If there is approval to proceed with this I will see what automated tests might make sense to add.

Screenshots

Before:

tcolour-before

After:

manual-text

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.

Fixes Automattic/wp-calypso#37051
Closes #18530

@glendaviesnz

This comment has been minimized.

Copy link
Contributor Author

glendaviesnz commented Dec 16, 2019

Adding this as a draft PR at this stage to see if there is interest in adding this to core. If there is interest then I will refine this, fix tests, documentation, etc.

@glendaviesnz glendaviesnz requested a review from youknowriad Dec 16, 2019
@glendaviesnz

This comment has been minimized.

Copy link
Contributor Author

glendaviesnz commented Dec 17, 2019

On thinking about this further, it is probably better to just go with the same colour setting options as the paragraph block rather than trying to automatically set a contrasting text colour, ie. provide both a text and background colour setting option, and warn the user if a low accessibility colour combination is chosen.

@glendaviesnz glendaviesnz force-pushed the try/group-block-custom-text-color branch from efae31d to 92b9e6f Dec 18, 2019
@glendaviesnz

This comment has been minimized.

Copy link
Contributor Author

glendaviesnz commented Dec 18, 2019

Pushed a revision which just adds a standard text colour selector, and refactors to use useColors instead of withColors. Would be good to get some feedback on:

  • Whether it is seen as a good idea to be able to set the text colour at the group block level
  • Whether it is better to just let the user manually do this, or to try and add some auto setting of a contrasting colour if a custom background colour us set
@karmatosed

This comment has been minimized.

Copy link
Member

karmatosed commented Dec 18, 2019

If I am right the design feedback here would be on whether to expect it to work without someone doing anything? For me, the answer is yes. I think it's fine as you an always go in and adjust yourself. I hope I am understanding this correctly though as to what the exact feedback is needed, if not please let me know.

@shaunandrews

This comment has been minimized.

Copy link

shaunandrews commented Dec 18, 2019

Text color for the group block makes sense to me.

Whether it is better to just let the user manually do this, or to try and add some auto setting of a contrasting colour if a custom background colour us set

Could we do both? Set a light text color if a dark background is select, but also let users choose a custom color - showing the contrast alert if needed.

@shaunandrews shaunandrews mentioned this pull request Dec 18, 2019
4 of 4 tasks complete
@richtabor

This comment has been minimized.

Copy link
Contributor

richtabor commented Dec 18, 2019

Just to clarify behavior: If you go add a color to the Paragraph block within a Group block, does that applied color override the Group block's applied TextColor? Or does the Group block's TextColor overrule any subsequently colors applied to text within it?

We've experimented with both applications on CoBlocks, but I lean towards letting subsequent blocks handle their color applications (or better yet, pass down color selections from parent blocks). This way, if you change the nested Paragraph block color, it affects the block itself.

Here's why:

If we went with a behavior where the Group block's TextColor wins every time, you end up with a UX scenario where you do not see the actual text color applied to the paragraph, even though the controls exist.

If we went with a behavior where the Group block's TextColor is secondary to applied colors within nested blocks, you could end up in a UX scenario where if you've applied colors to some nested blocks, but are left not knowing 100% if changing the TextColor on the Group block will actually change the color of the nested blocks.

Either way, it's not a fantastic experience. You actually end up having to mentally double-check that what you're doing will actually accomplish what you're wanting.

@ZebulanStanphill

This comment has been minimized.

Copy link
Contributor

ZebulanStanphill commented Dec 18, 2019

Logically, I think it makes sense for nested block controls to take precedence over the parent. Similar to how applying CSS styles work, a text color applied to a Paragraph should have a higher priority than the text color applied to its parent block.

I do think text controls on the Group block would be a good idea. It's not DRY (don't repeat yourself) to have to change all the Paragraphs in a section individually. Additionally, there's no guarantee that the nested blocks even have text color options (e.g. the List block currently lacks text color options). Actually, I think a Group block text color control is far more useful than a Paragraph text control.

Additionally, I think that since one of the primary uses of the Group block is to apply regional colors, we could surface both the background and text color options in the toolbar rather than the inspector. Remember that important block controls should not be hidden in the inspector.

@shaunandrews

This comment has been minimized.

Copy link

shaunandrews commented Dec 18, 2019

Similar to how applying CSS styles

Exactly this — it's the cascading bit of CSS. I see it making logical sense, and it maps to an existing pattern outside of Gutenberg (but very fundamental to the web.) Document editors like MS Word also do similar things.

@kwight

This comment has been minimized.

Copy link

kwight commented Dec 18, 2019

This works great @glendaviesnz ; I'd agree with others that adding the text controls and not letting the group block colors to clobber child block settings is the sensical way to go.

It is strange to click into a child block and its colors settings, and to see no active values when there are obviously active values from the group block. I wonder if even a notice similar to the contrast notice could explain to the user "These colors are currently determined by this block's Group Block. Setting colors here will override the Group Block's colors for this block." Uh, or something.

@shaunandrews

This comment has been minimized.

Copy link

shaunandrews commented Dec 18, 2019

Some sort of notice could be helpful. Though, I wonder if we'd have to show something for both background and text color on every child block that inherits colors from a parent? If so, seems like it could get a little too heavy.

image

@mapk

This comment has been minimized.

Copy link
Contributor

mapk commented Dec 18, 2019

It makes sense to me for the nested block's color selection to override the Group block's color selection. Maybe we can reduce some of the messaging by the scenario below:

Scenario:
I set the Group block's text color to be red.

If a nested Paragraph block does NOT have text color specified, then the text color turns red changing the nested Paragraph block's text color value to red. This way there's no messaging needed, I just see red as the selected color for my Paragraph block.

If a Paragraph block does have text color specified (green), then it remains green, overriding the Group block's text color. If I were to "clear" this Paragraph's text color, it would automatically inherit the Group block's text color setting.

So in this instance, the Group block's text color just passes that selection down to nested blocks and is reflected there as well. It works like a batch update.

@ZebulanStanphill

This comment has been minimized.

Copy link
Contributor

ZebulanStanphill commented Dec 18, 2019

@mapk How would that work for multiple levels of nesting? Also, how would that work with blocks that don't already have their own text color option (e.g. List, Latest Posts, etc.)? Also, moving the blocks out of the Group would cause their text color to remain the same as when they were in the Group, which seems wrong to me.

@gwwar

This comment has been minimized.

Copy link
Contributor

gwwar commented Dec 18, 2019

In terms of implementation will we be going with inline styles set on the elements? If so, this should handle cascading properly with arbitrarily nested elements. (Where if a child has a property set, this takes precedence over the parent.

The downside to this though, is that anything else attempting to override styling here would have trouble with the high CSS specificity (no number of classes or ids would be able to override).

@glendaviesnz

This comment has been minimized.

Copy link
Contributor Author

glendaviesnz commented Dec 19, 2019

Thanks for the great feedback everyone.

The second iteration of this PR works the way most are suggesting it should, ie.

  • Text colour selected on the group block applies to all child blocks, unless the child block has specified its own colour, in which case this overrides the parent colour
  • This is handled with default css cascading, so no style attributes are actually added to the child blocks - so if the child block is moved out of the current parent block the text colour will no longer be applied

Because of the fact that no attributes are applied to the children it will be difficult to show an indication that parent text colour styles are being applied without pushing down props through multiple levels of nested children, or some sore of redux plumbing.

It seems like a shame to lose the simplicity of css here, which does the bulk of what we want by default, by adding extra plumbing to show a parents style settings in a child components colour selector.

I am going to suggest that this PR is tidied up to just add the text colour option to the group, so at least people can set a suitable text colour for any background they select without having to manually set it on all the children. Then we could iterate back over this in further PRs to see if this can be easily indicated/messaged in child components, and if suitably contrasting text colours can be auto selected on custom background colour change.

@glendaviesnz

This comment has been minimized.

Copy link
Contributor Author

glendaviesnz commented Dec 19, 2019

Updated the description to better match the latest iteration and to include some of the feedback to-date.

@glendaviesnz glendaviesnz changed the title [WIP] Try/group block custom text color Try/group block custom text color Dec 19, 2019
@glendaviesnz glendaviesnz marked this pull request as ready for review Dec 19, 2019
@glendaviesnz

This comment has been minimized.

Copy link
Contributor Author

glendaviesnz commented Dec 19, 2019

Whether it is better to just let the user manually do this, or to try and add some auto setting of a contrasting colour if a custom background colour us set

Could we do both? Set a light text color if a dark background is select, but also let users choose a custom color - showing the contrast alert if needed.

Both would certainly be possible, but on looking at it, the new useColors hook seems like a better place to implement an automatic contrasting text selection option, rather than in the groups component specifically, that way it would be available to any component with useColours - have added a note about this to the PR description, in case someone wants to file a follow up issue about this.

@mapk

This comment has been minimized.

Copy link
Contributor

mapk commented Dec 20, 2019

How would that work for multiple levels of nesting?

It works just like I mentioned. The Group passes down the color if one isn't already set. I imagine if a user is setting the color on a Group block, their hoping to effect all text colors within it... unless some are already set. But if they wanted to effect those too, maybe there's a solution in the UI that allows this.

Also, how would that work with blocks that don't already have their own text color option (e.g. List, Latest Posts, etc.)?

Good question! In that case it would be helpful to show the Group block's text color option as this PR indicates.

Also, moving the blocks out of the Group would cause their text color to remain the same as when they were in the Group, which seems wrong to me.

It doesn't bother me to move a block that I purposefully changed text color on out of a Group, and have it keep the same color I already purposefully set on it. That feels alright to me.

@glendaviesnz

This comment has been minimized.

Copy link
Contributor Author

glendaviesnz commented Jan 8, 2020

@youknowriad, @talldan, @Soean, @ajitbohra, @jorgefilipecosta - before I rebase this PR, and maybe add some tests, are you able to confirm if this is feature that you would want added into core? Thanks.

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Jan 9, 2020

I feel like I'm missing context here. This has been discussed I believe when the block was first added. Maybe @getdave @talldan @mtias @kjellr and others could tell us why it wasn't added initially, if it was just left as a follow-up or if there was a strong argument against it.

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Jan 9, 2020

For what it's worth, I'm personally more of a fan of adding color controls to containers like this, rather than duplicating the same UI for each individual UI.

@glendaviesnz glendaviesnz force-pushed the try/group-block-custom-text-color branch from 92b9e6f to 6ccbe20 Jan 15, 2020
Glen Davies
@glendaviesnz

This comment has been minimized.

Copy link
Contributor Author

glendaviesnz commented Jan 15, 2020

I just rebased this, and updated the colorDetector to match the latest version. I have done some testing and it all seems to work as it should. If someone else had time to do some testing and review the code that would be great, thanks 😃

@talldan

This comment has been minimized.

Copy link
Contributor

talldan commented Jan 16, 2020

I don't recall any specific conversation about this on the original PR.

I had a quick look, but the PR is pretty huge (#13964).

The description mentions it was avoided to keep the original PR small, so probably just that.

@youknowriad youknowriad requested review from epiqueras and getdave Jan 16, 2020
@mapk

This comment has been minimized.

Copy link
Contributor

mapk commented Jan 17, 2020

I'm really stoked with how this is turning out! 👍 Here's a gif of how it looks/works.

groupcolors 2020-01-16 18_33_25

@glendaviesnz

This comment has been minimized.

Copy link
Contributor Author

glendaviesnz commented Jan 17, 2020

I'm really stoked with how this is turning out

Thanks for the detailed testing! People complain about CSS, but this is a scenario where the 'cascade' is our friend 😄

Copy link
Contributor

youknowriad left a comment

This is looking great for me. Thanks for the addition. I also love the refactor with useColors.

</div>
{ InspectorControlsColorPanel }
<BackgroundColor>
<TextColor>

This comment has been minimized.

Copy link
@youknowriad

youknowriad Jan 17, 2020

Contributor

It still seems that a single ApplyColors would be better as it's used that way in 90% of the use-cases. cc @epiqueras @jorgefilipecosta

This comment has been minimized.

Copy link
@epiqueras

epiqueras Jan 17, 2020

Contributor

It could be an additional component that's returned from the hook.

@richtabor

This comment has been minimized.

Copy link
Contributor

richtabor commented Jan 17, 2020

👍 Here's a gif of how it looks/works.

Looking good. One question though @mapk: With the gif as a reference, do you think it's confusing that the paragraph text within the group is "green", yet when you open its the Colors panel, the color is not reflected there?

Also, we need to verify these cascading styles are properly reflected on the frontend (using both color classes and custom inline styles).

@mapk

This comment has been minimized.

Copy link
Contributor

mapk commented Jan 17, 2020

With the gif as a reference, do you think it's confusing that the paragraph text within the group is "green", yet when you open its the Colors panel, the color is not reflected there?

Good question, @richtabor! The interaction here gets tricky. So currently when observing the color settings of the Paragraph block nested inside the Group block, we don't see the green color selected even though the Paragraph block has inherited green from the Group block color setting.

I'm okay with this interaction right now because I think it gets more complex if we choose to show the green selected color setting being pushed to the nested blocks too. Like if I change the Paragraph block to red, and then click to "clear" that color setting, does it default to green again? That doesn't feel like a proper "clear" to me. I'm willing to be persuaded otherwise.

I'd also suggest, that if the color selection does show for the nested blocks, then the nested blocks should keep that color selection when moved outside the Group block as well.

@mtias

This comment has been minimized.

Copy link
Contributor

mtias commented Jan 17, 2020

The interactions between these levels of color definition is precisely what the work of @ItsJonQ in #19611 aims to address. The "current color" on a paragraph being empty but still reflecting the color of a parent in the UI should be communicated somehow.

@glendaviesnz

This comment has been minimized.

Copy link
Contributor Author

glendaviesnz commented Jan 20, 2020

The "current color" on a paragraph being empty but still reflecting the color of a parent in the UI should be communicated somehow.

Any thoughts on whether we should merge it as is and circle back on this, or try and resolve this first?

@ZebulanStanphill

This comment has been minimized.

Copy link
Contributor

ZebulanStanphill commented Jan 20, 2020

I think this is fine to merge as-is.

@glendaviesnz glendaviesnz merged commit 5ba3976 into master Jan 20, 2020
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@glendaviesnz glendaviesnz deleted the try/group-block-custom-text-color branch Jan 20, 2020
@glendaviesnz

This comment has been minimized.

Copy link
Contributor Author

glendaviesnz commented Jan 20, 2020

Doh - apologies for the poor commit message, was focused on the more detailed message and overlooked that the first part was just the original branch name - I won't try a forced push to master just to try and amend this 😄

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.