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 🤔

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.

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?

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.

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

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

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

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!