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

Fix svg block icons #7140

Merged
merged 9 commits into from Jun 5, 2018

Conversation

Projects
None yet
5 participants
@jorgefilipecosta
Member

jorgefilipecosta commented Jun 5, 2018

The condition to check SVG icons was not correct. Now we use a different condition: if icon is defined, but the source is not defined we assume the icon should be the source. This avoids the need to check for all the possible icon sources e.g: string, function and React element.

Fixes #7139.
Regressed in #7068.

How has this been tested?

Go to existing blocks and change the icon to the following possibilities, verify in all the cases the result is the expected:

	icon: ( <svg width="20" height="20" viewBox="0 0 20 20">
		<circle cx="10" cy="10" r="10"
			fill="red" stroke="blue" strokeWidth="10" />
	</svg> ),
	icon: {
		background: '#f00',
		src: ( <svg width="20" height="20" viewBox="0 0 20 20">
			<circle cx="10" cy="10" r="10"
				fill="red" stroke="blue" strokeWidth="10" />
		</svg> ),
	},
	icon: () => ( <svg width="20" height="20" viewBox="0 0 20 20">
		<circle cx="10" cy="10" r="10"
			fill="red" stroke="blue" strokeWidth="10" />
	</svg> ),
Fix svg blocks svg icons
The condition to check SVG icons was not correct. Now we use the a different condition if icon is defined, but the source is not defined we assume the icon should be the source.

@jorgefilipecosta jorgefilipecosta changed the title from Fix svg blocks svg icons to Fix svg block svg icons Jun 5, 2018

@jorgefilipecosta jorgefilipecosta changed the title from Fix svg block svg icons to Fix svg block icons Jun 5, 2018

@jorgefilipecosta jorgefilipecosta requested a review from WordPress/gutenberg-core Jun 5, 2018

@youknowriad

Sorry for not catching that :(, This works for me, can we add a unit test?

@@ -66,7 +65,7 @@ export function normalizeIconObject( icon ) {
if ( ! icon ) {
return { src: 'block-default' };
}
if ( isString( icon ) || isFunction( icon ) || icon instanceof Component ) {
if ( ! icon.src ) {

This comment has been minimized.

@mcsf

mcsf Jun 5, 2018

Contributor

My concern with this is the lack of validation we provide. In contrast, registerBlockType does so much for block authors, but here we don't look at icon at all.

As also seen in BlockIcon, we have three types for WPIcon:

  • string
  • element
  • component taking zero arguments

Since there are no variable arguments at play, we can confidently test for the three cases:

  • typeof icon === 'string'
  • wp.element.isValidElement( icon )
  • wp.element.isValidElement( wp.element.createElement( icon ) )

If all fail, log an error during registerBlockType and bail, as for any other validation error in that function.

This requires exposing React's isValidElement in wp.element.

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Jun 5, 2018

Member

Hi @mcsf, thank you for the detailed feedback. I was not able to use
wp.element.isValidElement( wp.element.createElement( icon ) ), because if we pass something invalid to wp.element.createElement, the function throws an error, so we need to check if we are in the presence of a function or a Component.

jorgefilipecosta added some commits Jun 5, 2018

return { src: icon };
}
if ( ! isValidIcon( icon.src ) ) {
return null;

This comment has been minimized.

@gziolo

gziolo Jun 5, 2018

Member

This one is quite unexpected in my opinion. Shouldn't we return a default icon instead and warn?

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Jun 5, 2018

Member

This line is what makes sure that if a valid icon was not passed we will display an error as suggested by @mcsf. Without it, we never validate the icon.

This comment has been minimized.

@gziolo

gziolo Jun 5, 2018

Member

Shouldn't we split validation and normalization in that case? You would use it as follows inside blocks API:

if ( ! isValidIcon( settings.icon ) {
	console.error(
		'The icon passed is invalid.'
	);
	return;
}
settings.icon = normalizeIconObject( settings.icon );

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Jun 5, 2018

Member

Yes separating the concerns is a good idea. Validation and normalization were separated. We validate after normalization so even if the icon is in the format icon: { background: ..., src: ... } and the src is invalid we catch the problem.

it( 'should normalize the icon containing a string', () => {
const blockType = {
settingName: 'settingValue',
save: noop, category:

This comment has been minimized.

@gziolo

gziolo Jun 5, 2018

Member

Missing a new line before category.

it( 'should normalize the icon containing an element', () => {
const blockType = {
settingName: 'settingValue',
save: noop, category:

This comment has been minimized.

@gziolo

gziolo Jun 5, 2018

Member

category should go to the next line.

@jorgefilipecosta

This comment has been minimized.

Member

jorgefilipecosta commented Jun 5, 2018

Hi @youknowriad thank you for the review, it was not your fault I did a simple test passing a MyTestIcon component but totally missed the possibility of inline JSX while registering the icon. Test cases were added to the registration.

Hi @mcsf I added a function that validates if we have a valid icon.

const blockType = {
settingName: 'settingValue',
save: noop,
category:

This comment has been minimized.

@gziolo

gziolo Jun 5, 2018

Member

Is there something wrong with GitHub? I still see

category:
'common',

which should be simply:

category: 'common',

Eslint isn't very helpful, Prettier would make our job easier :(

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Jun 5, 2018

Member

They were corrected, thank you for the catch.

*
* @param {*} icon Parameter to be checked.
*
* @return {Boolean} True if the parameter is a valid icon and false otherwise.

This comment has been minimized.

@aduth

aduth Jun 5, 2018

Member

Boolean should be lowercase. Probably not reported as an error because of the newline between the JSDoc and the function.

*
* @param {Object} objectToCheck The object to be checked.
*
* @return {Boolean} true if objectToTest is a valid WPElement and false otherwise.

This comment has been minimized.

@aduth

aduth Jun 5, 2018

Member

Boolean should be lowercase. Probably not reported as an error because... well, I'm not sure 😄 Seems to be some issues with JSDoc used on export statements.

@@ -141,6 +141,12 @@ export function registerBlockType( name, settings ) {
}
settings.icon = normalizeIconObject( settings.icon );
if ( ! isValidIcon( settings.icon.src ) ) {
console.error(
'The icon passed is invalid.'

This comment has been minimized.

@aduth

aduth Jun 5, 2018

Member

We should provide a more helpful message about what constitutes a valid icon.

@@ -66,7 +83,7 @@ export function normalizeIconObject( icon ) {
if ( ! icon ) {
return { src: 'block-default' };
}
if ( isString( icon ) || isFunction( icon ) || icon instanceof Component ) {
if ( isValidIcon( icon ) ) {
return { src: icon };

This comment has been minimized.

@aduth

aduth Jun 5, 2018

Member

Not related to the issue, but don't we still want to proceed with icon.background logic below?

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Jun 5, 2018

Member

Not required because if "icon" is a valid icon it is a string or a React/element/component, and in that case, it should not contain a background color.

if ( ! isValidIcon( settings.icon.src ) ) {
console.error(
'The icon passed is invalid.' +
'The icon should be a string with a Slug of the DashIcon or an element (or function returning an element) if you choose to render your own SVG.' +

This comment has been minimized.

@aduth

aduth Jun 5, 2018

Member

Nitpicks:

  • There will be no space between this sentence and the last with the string concatenation.
    • Same issue with line below.
  • Slug is not a proper noun and does not need to be capitalized.
  • "DashIcon" should be capitalized as "Dashicon"

Given how many forms this field supports, I might regret suggesting the guidance 😄 Or, viewed in a different light, maybe it helps highlight that we have too many options. We could also simplify this to a brief overview of supported types (string, element, component class, function) with an external link to the documentation for more information.

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Jun 5, 2018

Member

I updated the string and linked to the docs. The previous text was a little bit big, this version is simpler. Space was added, it is a minor detail but would make the message hard to read.

'The icon passed is invalid.' +
'The icon should be a string with a Slug of the DashIcon or an element (or function returning an element) if you choose to render your own SVG.' +
'An object can also be passed, in this case, icon, as specified above, should be included in the src property.';
expect( console ).toHaveErroredWith( errorMsg );

This comment has been minimized.

@aduth

aduth Jun 5, 2018

Member

Minor: Not sure what we'd want to promote here, but with such complex message I'd lean more toward expect( console ).toHaveErrored() or, to avoid real errors, maybe toHaveErroredWith( expect.stringMatching( /* ... */ ) )

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Jun 5, 2018

Member

Thinking about it expect( console ).toHaveErrored() should be enough for testing purposes and allows to change the message without the need to update the tests.

</svg> ),
};
registerBlockType( 'core/test-block-icon-normalize-element', blockType );
blockType.mutated = true;

This comment has been minimized.

@mcsf

mcsf Jun 5, 2018

Contributor

This seems to be testing something else, correct? For the purpose of icon normalization, the question of whether a block type is frozen / mutated is not relevant. Or is the intent something else?

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Jun 5, 2018

Member

This tests that a copy of the blockType was made. We have a test case for that, I used it as a basis for my tests, and I missed the delete of this part. It is not relevant here and was removed. Thank you for the catch 👍

This comment has been minimized.

@mcsf

mcsf Jun 5, 2018

Contributor

Can we do the same for settingName/settingValue?

@mcsf

mcsf approved these changes Jun 5, 2018 edited

Don't forget my little last note about settingName, but after than you can :shipit:

@jorgefilipecosta jorgefilipecosta merged commit 1a7255f into master Jun 5, 2018

2 checks passed

codecov/project 46.48% (+0.09%) compared to 68ad658
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jorgefilipecosta jorgefilipecosta deleted the fix/svg-icons branch Jun 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment