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.

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

Add proper label to IconButton number element. #439

Merged
merged 3 commits into from
Apr 18, 2017

Conversation

jasmussen
Copy link
Contributor

The IconButton can optionally have a number indicating the level. For example heading might have this, quote has two styles, galleries might have multiple styles also. This PR adds a classname to indicate this, and moves the style to the iconbutton itself.

This intends to address feedback in #423 (comment) and #436 (comment).

The IconButton can optionally have a number indicating the level. For example heading might have this, quote has two styles, galleries might have multiple styles also. This PR adds a classname to indicate this, and moves the style to the iconbutton itself.

This intends to address feedback in #423 (comment) and #436 (comment).
@jasmussen jasmussen requested a review from aduth April 17, 2017 13:01
@aduth
Copy link
Member

aduth commented Apr 17, 2017

The IconButton can optionally have a number indicating the level.

Ignoring the editor context, I have a hard time agreeing that level is a natural feature of an icon button. I'll plan to poke at this a bit more.

@aduth
Copy link
Member

aduth commented Apr 17, 2017

Ignoring the editor context

I suppose this also depends on whether we care to enforce general reusability outside an editor context for these sorts of components.

@aduth
Copy link
Member

aduth commented Apr 17, 2017

Exploring a few ideas for extending an icon button with concept of level without necessarily building it into the button itself:

https://codepen.io/aduth/pen/eWNaoW

@jasmussen
Copy link
Contributor Author

Ooh, I dig the data-level approach.

@jasmussen
Copy link
Contributor Author

Put the icon level into a data-level attribute. Let me know what you think, thanks.

@ellatrix
Copy link
Member

Nice 👍

@jasmussen jasmussen merged commit 87e158b into master Apr 18, 2017
@jasmussen jasmussen deleted the add/icon-button-class branch April 18, 2017 09:22
@@ -14,9 +14,8 @@ function IconButton( { icon, children, label, className, ...additionalProps } )
const classes = classnames( 'editor-icon-button', className );

return (
<Button { ...additionalProps } aria-label={ label } className={ classes }>
<Button { ...additionalProps } aria-label={ label } className={ classes } data-level={ children ? children : null }>
Copy link
Contributor

Choose a reason for hiding this comment

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

The children prop can not be used as an attribute because it can contain other tags (components).
I think this should be updated to use a level prop.
We also need a children prop like the one defined before. It's being used in the Block switcher PR https://github.com/WordPress/gutenberg/pull/429/files#diff-4d78380ac51947daf9b826f57a1684bbR58

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I merged early here. Regardless, would appreciate some help with fixing it. 💖

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, I'll take care of that ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants