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 all 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
3 changes: 1 addition & 2 deletions packages/components/src/dashicon/icon-class.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
export const IconClass = ( props ) => {
const { icon, className } = props;
export const IconClass = ( icon, className ) => {
return [ 'dashicon', 'dashicons-' + icon, className ].filter( Boolean ).join( ' ' );
};
3 changes: 1 addition & 2 deletions packages/components/src/dashicon/icon-class.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
export const IconClass = ( props ) => {
const { icon, className, ariaPressed } = props;
export const IconClass = ( icon, className, ariaPressed ) => {
return [ ariaPressed ? 'dashicon-active' : 'dashicon', 'dashicons-' + icon, className ].filter( Boolean ).join( ' ' );
};
5 changes: 3 additions & 2 deletions 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, className, ariaPressed, ...extraProps } = this.props;
let path;

switch ( icon ) {
Expand Down Expand Up @@ -901,7 +901,7 @@ export default class Dashicon extends Component {
return null;
}

const iconClass = IconClass( this.props );
const iconClass = IconClass( icon, className, ariaPressed );
Copy link
Member

Choose a reason for hiding this comment

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

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


return (
<SVG
Expand All @@ -913,6 +913,7 @@ export default class Dashicon extends Component {
width={ size }
height={ size }
viewBox="0 0 20 20"
{ ...extraProps }
etoledom marked this conversation as resolved.
Show resolved Hide resolved
>
<Path d={ path } />
</SVG>
Expand Down
4 changes: 2 additions & 2 deletions packages/components/src/icon-button/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import Dashicon from '../dashicon';
// is common to apply a ref to the button element (only supported in class)
class IconButton extends Component {
render() {
const { icon, children, label, className, tooltip, shortcut, labelPosition, ...additionalProps } = this.props;
const { icon, children, label, className, tooltip, shortcut, labelPosition, iconProps, ...additionalProps } = this.props;
const { 'aria-pressed': ariaPressed } = this.props;
const classes = classnames( 'components-icon-button', className, {
'has-text': children,
Expand All @@ -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 }
Copy link
Member

Choose a reason for hiding this comment

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

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' } } /> } />

Copy link
Contributor Author

@etoledom etoledom Mar 26, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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!

{ children }
</Button>
);
Expand Down
2 changes: 1 addition & 1 deletion packages/components/src/toolbar-button/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,8 @@ function ToolbarButton( {
className,
isActive,
isDisabled,
extraProps,
Copy link
Member

Choose a reason for hiding this comment

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

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.

children,
...extraProps
} ) {
return (
<ToolbarButtonContainer className={ containerClassName }>
Expand Down