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.

Sign up for GitHub

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

Mobile: Dashicon color override #13977

Closed
wants to merge 12 commits into from
Closed

Mobile: Dashicon color override #13977

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
1452702
Mobile: Adding the posibility of override Dashicon default color
etoledom Feb 20, 2019
f60cb58
Mobile IconButton: Change color prop name.
etoledom Feb 20, 2019
63f18dc
Mobile Dashicon: Passing style prop instead of color directly.
etoledom Feb 20, 2019
c28b209
Dashicon: Simplify adding custom color and styles.
etoledom Feb 21, 2019
562b9aa
Merge branch 'rnmobile/release-v1.0' into rnmobile/add-block-icon-color
etoledom Feb 21, 2019
1523473
Merge branch 'rnmobile/release-v1.0' into rnmobile/add-block-icon-color
etoledom Feb 22, 2019
fc96acd
Dashicon: Passing props directly to SVG
etoledom Feb 22, 2019
c2fec4e
IconButton: Add iconProps to send to Dashicon component.
etoledom Feb 22, 2019
91dc3f6
Merge branch 'rnmobile/release-v1.0' into rnmobile/add-block-icon-color
etoledom Feb 22, 2019
b2a8e62
Fix unit tests errors on Dashicon.
etoledom Mar 5, 2019
f7ee93a
Small change to run CI
etoledom Mar 6, 2019
59b0824
Reverse last commit
etoledom Mar 6, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion packages/components/src/dashicon/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ export default class Dashicon extends Component {
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


render() {
const { icon, size = 20 } = this.props;
const { icon, size = 20, style, color } = this.props;
let path;

switch ( icon ) {
Expand Down Expand Up @@ -913,6 +913,8 @@ export default class Dashicon extends Component {
width={ size }
height={ size }
viewBox="0 0 20 20"
style={ style }
{ ...color && { color } }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update, I like this implementation better.

>
<Path d={ path } />
</SVG>
Expand Down
2 changes: 1 addition & 1 deletion packages/components/src/icon-button/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 }
Copy link
Member

@gziolo gziolo Feb 22, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

{ children }
</Button>
);
Expand Down