Mobile: Dashicon color override #13977

etoledom · 2019-02-20T16:34:46Z

Description

This PR adds the possibility to override the default Dashicons color. This is necessary to solve wordpress-mobile/gutenberg-mobile#546 .

I didn't find any already implemented way to do this, so I had to modify a couple of Gutenberg's index.js files. I'm not sure if this will affect Gutenberg Web, but local tests were passing.

Hopefully there's a better way!

To test:

Run the app.
Check that the add block button is filled with $bule-wordpress color.

aduth · 2019-02-20T20:10:15Z

packages/components/src/dashicon/index.js

@@ -913,6 +914,7 @@ export default class Dashicon extends Component {
 				width={ size }
 				height={ size }
 				viewBox="0 0 20 20"
+				{ ...colorProp }


It'd be simpler to express this as just color={ color }, skipping all of the colorProp conditions and object spread. I think it should be have exactly the same.

Further, it makes me wonder why not just pass through all additional props to the SVG.

Further further, these edits are made a generated file, where the source lives at https://github.com/WordPress/dashicons . See the note at the top of the file.

Thanks @aduth for the comments!

It'd be simpler to express this as just color={ color }

I did try that, but when color is undefined, it would remove the default color we are using for the rest of the icons. (for some reason)

With @Tug we decided to pass a style prop, and extract color from there, with the added advantage of passing more style props, that we are now taking advantage of. Please check the last PR :)

these edits are made a generated file

I was wondering about that. So I should edit https://github.com/WordPress/dashicons/tree/master/sources/react too 🤔

etoledom · 2019-02-21T17:59:41Z

Updated to add more clarity to the intention of the code.
Thank you @Tug !

Tug

Tested on gutenberg and gutenberg-mobile, this LGTM 🚢

youknowriad · 2019-02-22T09:34:47Z

packages/components/src/dashicon/index.js

@@ -913,6 +913,8 @@ export default class Dashicon extends Component {
 				width={ size }
 				height={ size }
 				viewBox="0 0 20 20"
+				style={ style }
+				{ ...color && { color } }


Can this be just { ...props } to avoid introducing a mobile specific prop to the web component?
The SVG is a component that have two different implementation, this is where the specific code should go right?

Thank you @youknowriad for the review!
I updated to pass all remaining props directly to SVG. Tested on mobile (iOS and Android) and it's behaving properly. 🎉

Could you check again please?
cc @Tug

Thanks for the update, I like this implementation better.

gziolo · 2019-02-22T12:52:04Z

packages/components/src/icon-button/index.js

@@ -45,7 +45,7 @@ class IconButton extends Component {

 		let element = (
 			<Button aria-label={ label } { ...additionalProps } className={ classes }>
-				{ isString( icon ) ? <Dashicon icon={ icon } ariaPressed={ ariaPressed } /> : icon }
+				{ isString( icon ) ? <Dashicon icon={ icon } ariaPressed={ ariaPressed } { ...additionalProps } /> : icon }


Can it cause any issues when the same props get passed down to Button and Dashicon?

This one should be tested:
https://github.com/WordPress/gutenberg/blob/master/packages/components/src/form-token-field/token.js#L72

It might apply aria-describedby twice which probably isn't expected.

Thank you @gziolo , that's a good concern.

Should we pass a specific iconProps?
I think it looks more declarative if we do this:

<Button aria-label={ label } { ...additionalProps } className={ classes }> { isString( icon ) ? <Dashicon icon={ icon } ariaPressed={ ariaPressed } { ...iconProps } /> : icon } { children } </Button>

From the caller:

<ToolbarButton label={ __( 'Add block' ) } icon="plus-alt" onClick={ onInsertClick } iconProps={ { style: styles.addBlockButton, color: styles.addBlockButton.color } } />

What other solution do you think might be good?

gziolo · 2019-02-24T15:34:00Z

There are 7 failing unit test:
https://travis-ci.org/WordPress/gutenberg/jobs/497085065#L5440

It looks like this needs some further debugging:

npm run test-unit

aduth · 2019-03-01T16:50:23Z

Does this help in any way avoid the Dashicon component being made aware of a "pressed" state, which does not seem a characteristic of an icon ?

#11827 (comment)

aduth · 2019-03-01T16:50:47Z

packages/components/src/dashicon/index.js

@@ -27,7 +27,7 @@ export default class Dashicon extends Component {
 	}


Immediately above this is a shouldComponentUpdate, which will prevent the component from being re-rendered even if we pass a new / different set of extraProps.

Right. Thank you for pointing that out!

Why are we doing this check with each prop?
Is there any special prop that we don't want it to make the component update?

Originally there were very few props, probably not more than just icon and size, and it was considered mostly a static component which would never need to be re-rendered.

Thanks for the explanation!
I'm still not sure what's the best approach to follow.
Currently I believe that we are re-rendering icons in some cases.

Would you recommend to remove those checks and let React do its job?
Or is there a good way of comparing extraProps directly?

Something like @wordpress/is-shallow-equal can provide some easier mechanism to compare all props:

shouldComponentUpdate( props, nextProps ) { return ! isShallowEqual( props, nextProps ); }

Which is effectively the same as using pure from @wordpress/compose.

But it's lost much of its value here. I don't know that it'd be necessary at all.

etoledom · 2019-03-06T07:42:31Z

There are 7 failing unit test:

@gziolo I have made changes to make the tests pass locally, but it seems like CI is not running here anymore. Any ideas?

etoledom · 2019-03-06T07:45:42Z

Does this help in any way avoid the Dashicon component being made aware of a "pressed" state, which does not seem a characteristic of an icon ?

@aduth I think this is a good step for that, since with these changes we can now set a color directly to dash icon passing a color prop on mobile.

gziolo · 2019-03-07T10:07:46Z

packages/components/src/toolbar-button/index.js

@@ -19,8 +19,8 @@ function ToolbarButton( {
 	className,
 	isActive,
 	isDisabled,
-	extraProps,


Just noting that extraProps is already in use in some places in Gutenberg:

gutenberg/packages/editor/src/components/block-settings-menu/index.js

Lines 46 to 57 in 17d509b

<Toolbar controls={ [ {

icon: 'ellipsis',

title: label,

onClick: () => {

if ( count === 1 ) {

onSelect( firstBlockClientId );

}

onToggle();

},

className: toggleClassname,

extraProps: { 'aria-expanded': isOpen },

} ] } />

So changes proposed will be braking for the components which depend on extraProps prop.

gziolo · 2019-03-07T10:13:16Z

packages/components/src/dashicon/index.js

@@ -901,7 +901,7 @@ export default class Dashicon extends Component {
 			return null;
 		}

-		const iconClass = IconClass( this.props );
+		const iconClass = IconClass( icon, className, ariaPressed );


IconClass - it looks like a name of the class but this is a regular function. It should be renamed to getIconClassName.

packages/components/src/dashicon/index.js

aduth · 2019-03-11T19:06:30Z

packages/components/src/icon-button/index.js

@@ -45,7 +45,7 @@ class IconButton extends Component {

 		let element = (
 			<Button aria-label={ label } { ...additionalProps } className={ classes }>
-				{ isString( icon ) ? <Dashicon icon={ icon } ariaPressed={ ariaPressed } /> : icon }
+				{ isString( icon ) ? <Dashicon icon={ icon } ariaPressed={ ariaPressed } { ...iconProps } /> : icon }


Out of curiosity, since icon can already be specified as an element for more advanced usage, had you considered just passing your own Dashicon element directly, rather than introduce a new separate prop iconProps?

Something like:

<IconButton icon={ <Dashicon icon="whatever" style={ { fill: 'red' } } /> } />

Thank you for this observation!
I didn't consider that option, but it does simplify the PR quite a bit.
There's a new PR that I have created to replace this one: #14631
Would you mind to take a look at it?

Thank you!

etoledom · 2019-03-26T10:53:01Z

Closing in favor of: #14631

etoledom added 2 commits February 20, 2019 16:57

Mobile: Adding the posibility of override Dashicon default color

1452702

Mobile IconButton: Change color prop name.

f60cb58

etoledom added the Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) label Feb 20, 2019

etoledom self-assigned this Feb 20, 2019

etoledom requested a review from Tug February 20, 2019 16:34

etoledom requested review from aduth, ajitbohra, chrisvanpatten, gziolo, jorgefilipecosta, noisysocks, talldan and youknowriad as code owners February 20, 2019 16:34

etoledom mentioned this pull request Feb 20, 2019

"Add Block" icon color wordpress-mobile/gutenberg-mobile#628

Merged

aduth requested changes Feb 20, 2019

View reviewed changes

etoledom added 3 commits February 20, 2019 21:12

Mobile Dashicon: Passing style prop instead of color directly.

63f18dc

Dashicon: Simplify adding custom color and styles.

c28b209

Merge branch 'rnmobile/release-v1.0' into rnmobile/add-block-icon-color

562b9aa

Tug approved these changes Feb 22, 2019

View reviewed changes

youknowriad reviewed Feb 22, 2019

View reviewed changes

etoledom added 2 commits February 22, 2019 12:35

Merge branch 'rnmobile/release-v1.0' into rnmobile/add-block-icon-color

1523473

Dashicon: Passing props directly to SVG

fc96acd

gziolo reviewed Feb 22, 2019

View reviewed changes

etoledom requested a review from gziolo February 22, 2019 13:35

etoledom added 2 commits February 22, 2019 16:10

IconButton: Add iconProps to send to Dashicon component.

c2fec4e

Merge branch 'rnmobile/release-v1.0' into rnmobile/add-block-icon-color

91dc3f6

aduth requested changes Mar 1, 2019

View reviewed changes

etoledom added 3 commits March 5, 2019 13:46

Fix unit tests errors on Dashicon.

b2a8e62

Small change to run CI

f7ee93a

Reverse last commit

59b0824

gziolo reviewed Mar 7, 2019

View reviewed changes

koke mentioned this pull request Mar 7, 2019

Rnmobile/native toolbar component ui #11827

Merged

2 tasks

aduth reviewed Mar 11, 2019

View reviewed changes

etoledom mentioned this pull request Mar 26, 2019

Send props to Dashicon. #14631

Merged

etoledom closed this Mar 26, 2019

aduth deleted the rnmobile/add-block-icon-color branch March 27, 2019 20:59

This was referenced Sep 6, 2019

[RNMobile] iOS DarkMode toolbar buttons #17356

Merged

iOS DarkMode toolbar buttons wordpress-mobile/gutenberg-mobile#1357

Merged

yassine4999 mentioned this pull request May 13, 2024

[Snyk] Security upgrade jest-snapshot from 27.5.1 to 29.5.0 yassine4999/gutenberg#43

Open

jay-Anonymous mentioned this pull request May 14, 2024

[Snyk] Security upgrade jest-snapshot from 27.5.1 to 29.5.0 jay-Anonymous/gutenberg#12

Open

npneeraj mentioned this pull request May 14, 2024

[Snyk] Security upgrade jest-snapshot from 27.5.1 to 29.5.0 npneeraj/gutenberg#40

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mobile: Dashicon color override #13977

Mobile: Dashicon color override #13977

etoledom commented Feb 20, 2019 •

edited

Loading

aduth Feb 20, 2019 •

edited

Loading

etoledom Feb 20, 2019 •

edited

Loading

etoledom commented Feb 21, 2019 •

edited

Loading

Tug left a comment

youknowriad Feb 22, 2019

etoledom Feb 22, 2019

youknowriad Feb 22, 2019

gziolo Feb 22, 2019 •

edited

Loading

etoledom Feb 22, 2019

gziolo commented Feb 24, 2019

aduth commented Mar 1, 2019

aduth Mar 1, 2019

etoledom Mar 5, 2019

aduth Mar 5, 2019

etoledom Mar 7, 2019

aduth Mar 11, 2019

etoledom commented Mar 6, 2019

etoledom commented Mar 6, 2019

gziolo Mar 7, 2019

gziolo Mar 7, 2019

aduth Mar 11, 2019

etoledom Mar 26, 2019 •

edited

Loading

etoledom commented Mar 26, 2019

		@@ -27,7 +27,7 @@ export default class Dashicon extends Component {
		}

	<Toolbar controls={ [ {
	icon: 'ellipsis',
	title: label,
	onClick: () => {
	if ( count === 1 ) {
	onSelect( firstBlockClientId );
	}
	onToggle();
	},
	className: toggleClassname,
	extraProps: { 'aria-expanded': isOpen },
	} ] } />

Mobile: Dashicon color override #13977

Mobile: Dashicon color override #13977

Conversation

etoledom commented Feb 20, 2019 • edited Loading

Description

aduth Feb 20, 2019 • edited Loading

Choose a reason for hiding this comment

etoledom Feb 20, 2019 • edited Loading

Choose a reason for hiding this comment

etoledom commented Feb 21, 2019 • edited Loading

Tug left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo Feb 22, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo commented Feb 24, 2019

aduth commented Mar 1, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etoledom commented Mar 6, 2019

etoledom commented Mar 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etoledom Mar 26, 2019 • edited Loading

Choose a reason for hiding this comment

etoledom commented Mar 26, 2019

etoledom commented Feb 20, 2019 •

edited

Loading

aduth Feb 20, 2019 •

edited

Loading

etoledom Feb 20, 2019 •

edited

Loading

etoledom commented Feb 21, 2019 •

edited

Loading

gziolo Feb 22, 2019 •

edited

Loading

etoledom Mar 26, 2019 •

edited

Loading