NavigationLink: improve colors handling by retrofox · Pull Request #20022 · WordPress/gutenberg

retrofox · 2020-02-04T04:55:35Z

Description

This PR improves the way of handling colors between the Navigation Block and its children (NavigationLink).

Background

The tricky parts about this handling are mainly based on the following points:

<NavigationItem /> blocks are created by the <InnerBlocks /> components.
It difficulties propagating data between the parent and the children blocks.
The way to share data is through the block attributes of the parent block.
Navigation colors are handled by the useColors hook.
The useColors hook provides colors in RGB format only if they are custom colors (when don't belong from colors palette).
NavigationLink always needs the colors in RGB format.

The way to get the color always in RGB is (master branch) getting this value from the color property of the component returned by the hook and store it as a block attribute in order to be able to pick it up from the children block.
This is a big mistake because storing and pulling the color to/from the block attribute makes the functionality static.

What this PR proposes is removing these additional attributes from the parent/Navigation block (rgbTextColor and rgbBlackgroundColor), and pick the RGB color by the color Id (when it comes from the colors palette), or just simply from the custom value if it's defined.

It makes the implementation simpler and clearer, avoiding to store unnecessary data in the Navigation attributes object.

Also, it fixes a bug when the text and/or background color are set using custom colors. The color attributes are not being saved correctly. This new refactoring implies also adding a deprecating handler since the rgbTextColor and rgbBackgrounColor attributes won't be used anymore.

How has this been tested?

Go to the editor canvas side
Create a Navigation Menu
Set a custom color for text and/or background
Save the post
Hard Refresh

before
6) Confirm that the colors are not applied
7) However, confirm that the colors are applied in the front-end

after
8) Confirm that the colors are applied in the canvas-editor after the hard-refresh

Props to @marekhrabe for the development of the idea. 👏

Testing deprecation

Go to master branch
Create a Navigation Menu
Set custom colors
Switch to the code editor

Confirm that they are applied using the RGB attributes

<!-- wp:navigation {"rgbTextColor":"#4296ba","rgbBackgroundColor":"#18333f"} -->
<!-- wp:navigation-link {"label":"Test"} /-->
<!-- /wp:navigation -->

Check colors are applied in the front-end
Switch to this branch
Check the colors are applied (again) in the front-end
In editor-canvas do a hard refresh
Check that the RGB attributes were removed and the custom ones are now there

<!-- wp:navigation {"customTextColor":"#4296ba","customBackgroundColor":"#18333f"} -->
<!-- wp:navigation-link {"label":"Test"} /-->
<!-- /wp:navigation -->

Checklist:

My code is tested.
My code follows the WordPress code style.
My code has proper inline documentation.
I've included developer documentation if appropriate.

getdave

Nice work here. Looks like it's heading in the right direciton.

The basic implementation as per your testing instructions worked and the custom colors were successfully propagated down from Nav > Nav Link and set as a style attribute in the DOM.

However, I noticed a potential coding issue where we're expecting the rootBlock to always be the Nav Block:

const rootBlock = getBlockParents( clientId )[ 0 ];

...when in actual fact I think it could actually be any parent Block that uses InnerBlocks (eg: Columms) as it will return the topmost Block in the hierarchy. Therefore if the Block is nested then this logic is problematic.

I tested this theory by:

Adding a Column Block
Add Nav Block within the first Column
Setting Custom Colors
Looking for style attribute set on the DOM on the Nav Link.

It appears in the above scenario it doesn't work as the style wasn't applied to the Navigation Link Block, only to the parent Navigation Block.

I could be wrong however, so please check and verify what I've highlighted.

Deprecations/Migrations

Also, I think we're going to need some deprecations for these changes. I created this Nav Block with custom colors on the master branch.

<!-- wp:navigation {"rgbTextColor":"#2898c8","rgbBackgroundColor":"#5a1534"} -->
<!-- wp:navigation-link {"label":"Sample Page","title":"Sample Page","type":"page","id":2,"url":"http://localhost:8889/?page_id=2"} /-->

<!-- wp:navigation-link {"label":"Special page","title":"Special page","type":"page","id":8,"url":"http://localhost:8889/?page_id=8"} /-->

<!-- wp:navigation-link {"label":"hello","title":"hello","type":"page","id":11,"url":"http://localhost:8889/?page_id=11"} /-->

<!-- wp:navigation-link {"label":"fake","title":"fake","type":"page","id":15,"url":"http://localhost:8889/?page_id=15"} /-->

<!-- wp:navigation-link {"label":"fake","title":"fake","type":"page","id":16,"url":"http://localhost:8889/?page_id=16"} /-->

<!-- wp:navigation-link {"label":"fake","title":"fake","type":"page","id":17,"url":"http://localhost:8889/?page_id=17"} /-->

<!-- wp:navigation-link {"label":"fake","title":"fake","type":"page","id":18,"url":"http://localhost:8889/?page_id=18"} /-->

<!-- wp:navigation-link {"label":"fake","title":"fake","type":"page","id":19,"url":"http://localhost:8889/?page_id=19"} /-->

<!-- wp:navigation-link {"label":"fake","title":"fake","type":"page","id":20,"url":"http://localhost:8889/?page_id=20"} /-->

<!-- wp:navigation-link {"label":"__CREATE__","title":"__CREATE__","type":"page","id":21,"url":"http://localhost:8889/?page_id=21"} /-->
<!-- /wp:navigation -->

Try pasting this into Code mode and then switching back to "Visual Mode". You'll see the custom colours are not carried across between Block versions.

getdave · 2020-02-04T13:31:23Z

packages/block-library/src/navigation-link/edit.js

 		const showSubmenuIcon =
-			!! navigationBlockAttributes.showSubmenuIcon && hasDescendants;
+			!! navBlockAttrs.showSubmenuIcon && hasDescendants;
 		const isParentOfSelectedBlock = hasSelectedInnerBlock( clientId, true );


Minor: in reality - due to the the 3rd true arg triggering "deep" mode - this [isParentOfSelectedBlock ] is actually determining if it's an ancestor of the selected Block.

packages/block-library/src/navigation-link/edit.js

packages/block-library/src/navigation/index.php

retrofox · 2020-02-04T18:16:47Z

However, I noticed a potential coding issue where we're expecting the rootBlock to always be the Nav Block:
const rootBlock = getBlockParents( clientId )[ 0 ];

Nice catch, Dave. In order to fix this issue, I've added a new selected name getBlockParentsByBlockName(), which returns a client IDs array but filtering by block name. IN this way, we ensure to get the proper parent block:

const rootBlock = getBLockParentsByBlockName(
	clientId,
	'core/navigation'
)[ 0 ];

retrofox · 2020-02-04T19:39:09Z

However, I noticed a potential coding issue where we're expecting the rootBlock to always be the Nav Block:
const rootBlock = getBlockParents( clientId )[ 0 ];
...when in actual fact I think it could actually be any parent Block that uses InnerBlocks (eg: Columms) as it will return the topmost Block in the hierarchy. Therefore if the Block is nested then this logic is problematic.

Thanks, @getdave for the review. The issue is gonna be handled in #20032. I've copied part of your text for the PR description 😅

packages/block-editor/src/store/selectors.js

packages/block-library/src/navigation-link/edit.js

packages/block-library/src/navigation/index.php

packages/block-library/src/navigation/depreated.js

jorgefilipecosta · 2020-02-04T22:31:26Z

packages/block-editor/src/store/selectors.js

Hi @retrofox,
Selectors are part of the public API, I would prefer to avoid adding these functions to the public API.
As a temporary solution, you may query the values outside a selector relying on getSettings. But ideally, we would use useColors in this case, I'm not sure if it is possible because it seems we relly on the colors of the parent.

cc: @epiqueras Do you think is there a way to use the useColors hook in this case to avoid the need to query colors by slug?

Yes. The components returned by the hook have a color property that always contains the color value, even for preset colors.

We weren't able to use useColors directly here because it would try to read color attributes from attributes of the current block. In this case it is always empty because those values actually come from one of the parent blocks. Or am I misreading the usage?

The parent block Navigation has useColors implemented but this usage is a bit different

The tough point here is how to propagate the colors from the Navigation to its children (NavLink).
Right now, we are pulling the attrs from the children, and it works as expected.
The colors are handled by the useColors() hook straight in the parent Navigation block, and it's great.

The issue is we need always the color in RGB format, and it doesn't happen right now since the custom color will be undefined if the color was picked up from colors palette.

It true that we can pick up the color in RGB through of the .color the property, but then we need to store this new (lets say rgbColor) attribute into the Navigation in order to be able to pull it from its children. In fact, it's working this way now in master.

For all of this, we suggest removing this attribute and compute the color in the children. I'm going to move this selector outside of the public API.

packages/block-library/src/navigation/index.js

jorgefilipecosta · 2020-02-04T22:37:14Z

packages/block-library/src/navigation-link/edit.js

Should we name the property customTextColor?

I'm not 100% sure on this because the custom… prefix is already established as a key for the hex color value but ONLY when color is not a swatch. If the selected color is a named swatch, the custom… will be undefined. This rgb however ALWAYS holds a value, regardless of its source. Does it make sense?

yes, also the custom${ colorName } is automatically handled by the useColors hook, and it could get confusing if we define the same name structure for this arbitrary property.

jorgefilipecosta · 2020-02-04T22:40:59Z

packages/block-library/src/navigation-link/edit.js

Do the navigation items need to explicitly set the colors to have the same values as the parent? Could they have styles that inherit the color?

I remember there have been problems with inheritance and not having enough power to override CSS rules of many themes. Bringing values inline this way resolved most of them. Maybe somebody else can explain more.

The long run would be to possibly move to CSS variables but we cannot do it yet because of remaining IE support.

Yes, it's necessary to apply the styles rightly to navigation links. Sometimes it isn't, depending on the type of color applied (from colors palette or custom one), or the theme, etc.
CSS vars simplifies this enormously, but again it isn't supported by IE.

Do the navigation items need to explicitly set the colors to have the same values as the parent? Could they have styles that inherit the color?

We apply the CSS class and inline style to each NavigationLink as the Navigation does. In some way, it follows up the behavior of the useColors() hook through its HOCs.
Also, it's aligned with the idea of being able to set up specific colors for each item.

@retrofox

According to @retrofox the review was addressed.

packages/block-library/src/navigation-link/edit.js

packages/block-library/src/navigation/index.php

navigation-link: get color from slug or value navigation-link: rename prop to improve reading navigation: remove stuff about rgb block attrs navigation-link: replace getting rgb color values navigation: fixing linting errors minor doc improvements navigation-link: fix default colors value block-editor: add getColors() selector block-editor: add getColorObjectByColorSlug() It adds the getColorObjectByColorSlug() selector. block-editor: remove colors helper function navigation-edit: refact using selectors navigation-link: set nav colors as single props docs: add getColorObjectByColorSlug() selector doc navigation: handling deprecated attrs navigation: adding support to deprecated navigation: handling deprecated in server side navigation: fixing linting errors navigation-link: remove wrong comment navigation: fix typo in file name navigation: getting rbg color with local fn block-editor: remove color selectors navigation: simplify deprecated in server-side

marekhrabe · 2020-02-06T02:25:47Z

My concerns are addressed now and I think the last question remaining is the reason why we shouldn't make selectors for colors public - are they temporary? unstable? or too internal?

We can easily move the code from the public surface but it would be good to know the reason so we can follow it next time. cc: @jorgefilipecosta

retrofox · 2020-02-06T03:04:29Z

why we shouldn't make selectors for colors public - are they temporary? unstable? or too internal?

Let me give my opinion beyond that Jorge surely can give us a better one.
The proper way to deal with colors always should be using the useColors() hook as default. Although it' still experimental (I'm not totally sure, though of this), we always should use it as the primary option.
Saying this, we can conclude that adding color selectors, especially in the public API, could bring us to implementations not desired, generating confusion and in some way losing the horizon of a correct implementation.
We need to keep in mind also that the implementation of the Navigation is also particular for the fact that it composes their children (Navigation Links) using the InnerBlocks component, getting the communication between components tricky.

WunderBart

Tested locally. Colors handling improved indeed! Thanks 🙌

retrofox added [Type] Bug An existing feature does not function as intended [Type] Enhancement A suggestion for improvement. [Block] Navigation Affects the Navigation Block labels Feb 4, 2020

retrofox requested review from getdave and marekhrabe February 4, 2020 04:55

retrofox requested review from Soean, ajitbohra, ellatrix, jorgefilipecosta, talldan and youknowriad as code owners February 4, 2020 04:55

retrofox force-pushed the update/navigation-improve-getting-rgb-colors branch 4 times, most recently from c2d08b8 to 85d7a01 Compare February 4, 2020 13:37

retrofox requested review from chrisvanpatten and mkaz as code owners February 4, 2020 13:37

retrofox force-pushed the update/navigation-improve-getting-rgb-colors branch from 85d7a01 to 54fd27a Compare February 4, 2020 13:45

getdave previously requested changes Feb 4, 2020

View reviewed changes

retrofox force-pushed the update/navigation-improve-getting-rgb-colors branch from 53fecc4 to fbe586a Compare February 4, 2020 18:15

retrofox force-pushed the update/navigation-improve-getting-rgb-colors branch from fbe586a to b5763f4 Compare February 4, 2020 18:19

retrofox mentioned this pull request Feb 4, 2020

NavigationLink: fix getting Navigation parent block #20032

Merged

5 tasks

retrofox force-pushed the update/navigation-improve-getting-rgb-colors branch 2 times, most recently from b0da932 to dd6531e Compare February 4, 2020 19:08

retrofox force-pushed the update/navigation-improve-getting-rgb-colors branch from dd6531e to ec7161b Compare February 4, 2020 19:41

marekhrabe requested a review from getdave February 4, 2020 19:50

marekhrabe reviewed Feb 4, 2020

View reviewed changes

packages/block-editor/src/store/selectors.js Outdated Show resolved Hide resolved

marekhrabe reviewed Feb 4, 2020

View reviewed changes

packages/block-library/src/navigation-link/edit.js Outdated Show resolved Hide resolved

retrofox force-pushed the update/navigation-improve-getting-rgb-colors branch from aeaea58 to 43024f6 Compare February 4, 2020 21:17

obenland reviewed Feb 4, 2020

View reviewed changes

packages/block-library/src/navigation/index.php Outdated Show resolved Hide resolved

marekhrabe reviewed Feb 4, 2020

View reviewed changes

packages/block-library/src/navigation/depreated.js Outdated Show resolved Hide resolved

jorgefilipecosta reviewed Feb 4, 2020

View reviewed changes

jorgefilipecosta requested a review from epiqueras February 4, 2020 22:32

marekhrabe reviewed Feb 4, 2020

View reviewed changes

packages/block-library/src/navigation/index.js Outdated Show resolved Hide resolved

jorgefilipecosta reviewed Feb 4, 2020

View reviewed changes

retrofox force-pushed the update/navigation-improve-getting-rgb-colors branch from 06ca912 to e5ca223 Compare February 5, 2020 17:57

retrofox force-pushed the update/navigation-improve-getting-rgb-colors branch 4 times, most recently from 89f0f20 to 0b31b95 Compare February 5, 2020 22:50

obenland reviewed Feb 5, 2020

View reviewed changes

packages/block-library/src/navigation-link/edit.js Outdated Show resolved Hide resolved

packages/block-library/src/navigation/index.php Outdated Show resolved Hide resolved

retrofox added 3 commits February 5, 2020 18:20

navigation: swtich commenting block using *

90fdad6

navigation-link: avoid abbreviations in var name

7aabbbf

retrofox force-pushed the update/navigation-improve-getting-rgb-colors branch from 81c3f17 to 7aabbbf Compare February 6, 2020 02:20

WunderBart approved these changes Feb 6, 2020

View reviewed changes

retrofox merged commit cbda33e into master Feb 6, 2020

retrofox deleted the update/navigation-improve-getting-rgb-colors branch February 6, 2020 14:09

github-actions bot added this to the Gutenberg 7.5 milestone Feb 6, 2020

Conversation

retrofox commented Feb 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Background

How has this been tested?

Testing deprecation

Checklist:

Uh oh!

getdave left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Deprecations/Migrations

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

retrofox commented Feb 4, 2020

Uh oh!

retrofox commented Feb 4, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

retrofox Feb 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

marekhrabe commented Feb 6, 2020

Uh oh!

retrofox commented Feb 6, 2020

Uh oh!

WunderBart left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

retrofox commented Feb 4, 2020 •

edited

Loading

getdave left a comment •

edited

Loading

retrofox Feb 5, 2020 •

edited

Loading