Add a generic style to the toolbar buttons #21252
Conversation
youknowriad
added
the
[Feature] UI Components
youknowriad
requested review from
ajitbohra,
chrisvanpatten,
ellatrix and
jorgefilipecosta
as code owners
|
Size Change: -28 B (0%) Total Size: 883 kB
ℹ️ View Unchanged
|
|
I love the simplicity of this fix, it's definitely more solid and future-proof code. Two things though. The caption inline toolbar is now vertical — probably needs a flex rule it previously inherited:
It might also have inherited other rules that are good to check for. For example the inline buttons in the middle should be 36px wide to optically be balanced. Here's how it's supposed to look:
The other thing is the styling of the "toolbar" when shown in the sidebar. It feels most correct to have the same toolbar styling as exists in the canvas, like so:
Note that the icons above are 20x20 but should be 24x24, so that's a bug in the above which is correct in your branch. |
I switched to inline-flex because we don't want to default toolbar to take the full width available, especially cause it has borders but I wonder if I should revert that change.
That's the one thing I'm unsure of. The question here is what is the default height for toolbars (regardless of whether they are in blocks or not). Maybe that's 48 (like the block one) and in that case, the fix would be even simpler. The problem is its inconsistency with buttons but maybe it's fine? |
Inline-flex should be able to have horizontal buttons inside no? flex-direction: row?
It should be, I would say. Do you foresee problems with this? |
It should by default, not sure why the wrapping happens yet.
Not really, just design-wise, it might not be perfect. |
Just the duplicated toolbar in the sidebar? Yeah it's a little mixing of metaphors, but it seems like the strongest path forward for now. |
youknowriad
force-pushed
the
update/toolbar-button-styles
branch
from
9852031
to
cbdbf54
Compare
|
I applied the styles to all toobars, so now you can see the styles in Storybook and I added a story with several variations.
It is becoming an impactful PR though so we need to test well. there might be some dead CSS code as well lingering. |
|
Wow, that last commit is seriously impressive. This feels like an excellent step in the right direction. This also tested well for me, I tested the demo content and even some plugins. Everything seems to work as it should. I would very much appreciate others also testing, so we can be sure we've dusted off all the corners this might impact, but it feels really worth it. So I have only one thing:
The toolbar should have rounded corners. |
|
In addition to what @jasmussen noticed, one inconsistency I noticed is that the label for the toolbar uses different font styles than other controls. (Not sure if that's how it is in
Other than that, this looks good to me! |
|
Added the rounded borders. The label was not touched in this PR but yes, it seems like it's not like the remaining ones. |
|
Looks like this is now conflicting with the focus changes you just merged. Would you mind helping with the rebase @jasmussen |
|
I'll take a look in a moment. |
jasmussen
force-pushed
the
update/toolbar-button-styles
branch
from
6b99f2e
to
9642fb3
Compare
|
Okay, rebased! |
|
Thanks :) |
|
Is this ready to land ? |
|
Last I checked, absolutely. |
| /> | ||
| { children } | ||
| > | ||
| { children } |
There was a problem hiding this comment.
@youknowriad Is it possible that this now overrides what's being passed in extraProps.children?
If you passed a button text to the toolbar previous to 7.9 that button now appears empty: Automattic/jetpack#15459
There was a problem hiding this comment.
For me the change here is not to avoid passing "children" but moving the children position inside the button which makes more sense IMO
There was a problem hiding this comment.
That said, I'm still wondering why it regressed, the children should still show up for toolbar buttons? 🤔
There was a problem hiding this comment.
It looks like the "inner" children supersedes the children that is passed as a prop?
We fixed it in Automattic/jetpack#15508 through emulating how MediaReplaceFlow does it, so don't feel inclined to change anything on behalf of this. Awareness might be enough in case others run into this at some point
There was a problem hiding this comment.
We can also switch the priorities of the passed children (give the extraProps more priority), it is also weird to have the same prop in two different places, we should consolidate maybe. I wonder where the "extraProp" prop is passed and in which case the children should be extracted and just passed as regular "children" prop.
|
Is this the Issue? When i add the Imageblock the Fontbar hidden the Text. |
|
@RickorDD yes, seems like a bug. Would you mind creating an issue? It seems the caption input should be hidden unless the image has been selected. |
|
@youknowriad |
Alternative to #21064
Related to #20246
Right now the toolbar component has an isPressed state for buttons but the styles are not applied properly. Instead the styles are only applied for block toolbars.
This PR fixes that by providing default styles (based on the regular button sizes) and the block toolbar just overrides the height of these toolbars.
Testing instructions
Check the heading level toolbar on the inspector.