Conversation
|
Looks great!
Really ties the room together. |
|
Notes:
|
blocks/library/heading/index.js
Outdated
There was a problem hiding this comment.
Why do we need [ ...array ]. We could just use array
There was a problem hiding this comment.
Using children for this is more appropriate to me, don't you think?
There was a problem hiding this comment.
Could you give an example?
There was a problem hiding this comment.
The exact same thing, just replace text with children in order to use this like this:
<IconButton>MyText</IconButton>
I've done something similar in #429 but without the span(I don't mind keeping the span)
There was a problem hiding this comment.
I'm not sure I understand. Could you give an example?
There was a problem hiding this comment.
function IconButton( { icon, children, label, className, ...additionalProps } ) {
// ...
{ children ? <span>{ children }</span> : null } // This replaces the line 19
// ..
}And now instead of writing <IconButton text="My Text" />, you'd write <IconButton>My Text</IconButton>.
There was a problem hiding this comment.
Ah I see :) Thought you meant writing { text ? <span>{ text }</span> : null } differently.
99344e9 to
8502267
Compare
|
@joen I can't seem to get the heading level text just right. |
I'll take a look! |
youknowriad
left a comment
There was a problem hiding this comment.
👍 Nice work. The sizing toolbar needs We could use some design ❤️ but this is good enough for me.
|
Feel free to merge in, I'll take a look at the numbers in a separate PR. |
| return ( | ||
| <Button { ...additionalProps } aria-label={ label } className={ classes }> | ||
| <Dashicon icon={ icon } /> | ||
| { children ? <span>{ children }</span> : null } |
There was a problem hiding this comment.
Do we need the span wrapper on children?
| } | ||
|
|
||
| span { | ||
| font-size: 12px; |
There was a problem hiding this comment.
This styling seems very targeted at the heading use-case. Is IconButton with small adjacent text a common enough pattern it should be engrained into the IconButton component itself?
There was a problem hiding this comment.
Great question to be asking. The icon-button should be just that, an icon button, so any text added should not so much be a label as it should be something to go with the icon itself. In this case, implying a heading size. But it could also be used for quote styles, or possibly even gallery styles (i.e. quote 2 or gallery 3).
Tying together with your comment #436 (review), I will open a PR to add a label that makes this clear.
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).
| ...'123456'.split( '' ).map( ( level ) => ( { | ||
| icon: 'heading', | ||
| text: level, | ||
| title: wp.i18n.sprintf( wp.i18n.__( 'Heading %s' ), level ), |
There was a problem hiding this comment.
Should we use %d placeholder instead of %s?
There was a problem hiding this comment.
Sounds good too. Does it matter much? Sorry for my ignorance.
There was a problem hiding this comment.
Sounds good too. Does it matter much? Sorry for my ignorance.
I'll not claim to be a localization expert, but perhaps as a hint to the translator that it's expected to be translated with a number vs. any arbitrary string could affect the translated text. Also prevents said arbitrary strings if we're very intentional that it's not to be expected (as a result of some potentially buggy code).
See #313.
Props @BE-Webdesign for initial PR in #337.