Add a generic style to the toolbar buttons #21252

youknowriad · 2020-03-30T09:32:49Z

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.

github-actions · 2020-03-30T09:38:45Z

Size Change: -28 B (0%)

Total Size: 883 kB

Filename	Size	Change
`build/block-editor/index.js`	102 kB	-9 B (0%)
`build/block-editor/style-rtl.css`	10.7 kB	-496 B (4%)
`build/block-editor/style.css`	10.7 kB	-492 B (4%)
`build/components/style-rtl.css`	16.6 kB	+506 B (3%)
`build/components/style.css`	16.5 kB	+499 B (3%)
`build/edit-post/style-rtl.css`	12 kB	-9 B (0%)
`build/edit-post/style.css`	12 kB	-9 B (0%)
`build/edit-site/style-rtl.css`	4.61 kB	-9 B (0%)
`build/edit-site/style.css`	4.61 kB	-9 B (0%)

ℹ️

View Unchanged

Filename	Size	Change
`build/a11y/index.js`	1.02 kB	0 B
`build/annotations/index.js`	3.45 kB	0 B
`build/api-fetch/index.js`	3.8 kB	0 B
`build/autop/index.js`	2.59 kB	0 B
`build/blob/index.js`	620 B	0 B
`build/block-directory/index.js`	6.02 kB	0 B
`build/block-directory/style-rtl.css`	760 B	0 B
`build/block-directory/style.css`	760 B	0 B
`build/block-library/editor-rtl.css`	7.21 kB	0 B
`build/block-library/editor.css`	7.21 kB	0 B
`build/block-library/index.js`	110 kB	0 B
`build/block-library/style-rtl.css`	7.5 kB	0 B
`build/block-library/style.css`	7.51 kB	0 B
`build/block-library/theme-rtl.css`	669 B	0 B
`build/block-library/theme.css`	671 B	0 B
`build/block-serialization-default-parser/index.js`	1.65 kB	0 B
`build/block-serialization-spec-parser/index.js`	3.1 kB	0 B
`build/blocks/index.js`	57.5 kB	0 B
`build/components/index.js`	195 kB	0 B
`build/compose/index.js`	6.21 kB	0 B
`build/core-data/index.js`	10.7 kB	0 B
`build/data-controls/index.js`	1.03 kB	0 B
`build/data/index.js`	8.23 kB	0 B
`build/date/index.js`	5.37 kB	0 B
`build/deprecated/index.js`	772 B	0 B
`build/dom-ready/index.js`	569 B	0 B
`build/dom/index.js`	3.05 kB	0 B
`build/edit-navigation/index.js`	2.4 kB	0 B
`build/edit-navigation/style-rtl.css`	95 B	0 B
`build/edit-navigation/style.css`	95 B	0 B
`build/edit-post/index.js`	92.3 kB	0 B
`build/edit-site/index.js`	9.04 kB	0 B
`build/edit-widgets/index.js`	4.43 kB	0 B
`build/edit-widgets/style-rtl.css`	3.74 kB	0 B
`build/edit-widgets/style.css`	3.74 kB	0 B
`build/editor/editor-styles-rtl.css`	423 B	0 B
`build/editor/editor-styles.css`	426 B	0 B
`build/editor/index.js`	42.8 kB	0 B
`build/editor/style-rtl.css`	3.47 kB	0 B
`build/editor/style.css`	3.47 kB	0 B
`build/element/index.js`	4.44 kB	0 B
`build/escape-html/index.js`	733 B	0 B
`build/format-library/index.js`	6.94 kB	0 B
`build/format-library/style-rtl.css`	502 B	0 B
`build/format-library/style.css`	502 B	0 B
`build/hooks/index.js`	1.93 kB	0 B
`build/html-entities/index.js`	622 B	0 B
`build/i18n/index.js`	3.57 kB	0 B
`build/is-shallow-equal/index.js`	710 B	0 B
`build/keyboard-shortcuts/index.js`	2.3 kB	0 B
`build/keycodes/index.js`	1.7 kB	0 B
`build/list-reusable-blocks/index.js`	2.99 kB	0 B
`build/list-reusable-blocks/style-rtl.css`	226 B	0 B
`build/list-reusable-blocks/style.css`	226 B	0 B
`build/media-utils/index.js`	4.84 kB	0 B
`build/notices/index.js`	1.57 kB	0 B
`build/nux/index.js`	3.01 kB	0 B
`build/nux/style-rtl.css`	616 B	0 B
`build/nux/style.css`	613 B	0 B
`build/plugins/index.js`	2.54 kB	0 B
`build/primitives/index.js`	1.5 kB	0 B
`build/priority-queue/index.js`	780 B	0 B
`build/redux-routine/index.js`	2.84 kB	0 B
`build/rich-text/index.js`	14.5 kB	0 B
`build/server-side-render/index.js`	2.54 kB	0 B
`build/shortcode/index.js`	1.7 kB	0 B
`build/token-list/index.js`	1.28 kB	0 B
`build/url/index.js`	4.01 kB	0 B
`build/viewport/index.js`	1.6 kB	0 B
`build/warning/index.js`	1.14 kB	0 B
`build/wordcount/index.js`	1.17 kB	0 B

_{compressed-size-action}

jasmussen · 2020-03-30T10:07:17Z

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.

youknowriad · 2020-03-30T10:20:30Z

Two things though. The caption inline toolbar is now vertical — probably needs a flex rule it previously inherited:

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.

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:

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?

jasmussen · 2020-03-30T10:23:41Z

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.

Inline-flex should be able to have horizontal buttons inside no? flex-direction: row?

The question here is what is the default height for toolbars
Maybe that's 48 (like the block one) and in that case, the fix would be even simpler

It should be, I would say. Do you foresee problems with this?

youknowriad · 2020-03-30T10:35:06Z

Inline-flex should be able to have horizontal buttons inside no? flex-direction: row?

It should by default, not sure why the wrapping happens yet.

Do you foresee problems with this?

Not really, just design-wise, it might not be perfect.

jasmussen · 2020-03-30T12:39:14Z

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 · 2020-03-30T13:00:35Z

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.

jasmussen · 2020-03-30T13:36:39Z

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. border-radius: $radius-block-ui;.

ZebulanStanphill · 2020-03-30T15:27:30Z

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 master, but it's not like that in Gutenberg 7.8.1.)

Other than that, this looks good to me!

youknowriad · 2020-03-31T09:05:28Z

Added the rounded borders.

The label was not touched in this PR but yes, it seems like it's not like the remaining ones.

youknowriad · 2020-04-01T08:28:36Z

Looks like this is now conflicting with the focus changes you just merged. Would you mind helping with the rebase @jasmussen

jasmussen · 2020-04-01T08:29:50Z

I'll take a look in a moment.

jasmussen · 2020-04-01T08:58:30Z

Okay, rebased!

youknowriad · 2020-04-01T09:10:53Z

Thanks :)

youknowriad · 2020-04-02T12:54:13Z

Is this ready to land ?

jasmussen · 2020-04-02T13:01:04Z

Last I checked, absolutely.

obenland · 2020-04-20T23:21:01Z

packages/components/src/toolbar-button/index.js

-				/>
-				{ children }
+				>
+					{ children }


@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

For me the change here is not to avoid passing "children" but moving the children position inside the button which makes more sense IMO

That said, I'm still wondering why it regressed, the children should still show up for toolbar buttons? 🤔

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

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.

RickorDD · 2020-06-07T09:31:21Z

Is this the Issue?

When i add the Imageblock the Fontbar hidden the Text.
Remove this Bar to the Top?

youknowriad · 2020-06-08T08:32:17Z

@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.

RickorDD · 2020-06-08T13:34:37Z

@youknowriad
We found this Issue with
#22295

youknowriad added the [Feature] UI Components label Mar 30, 2020

youknowriad requested review from jasmussen, earnjam and ZebulanStanphill Mar 30, 2020

youknowriad requested review from ajitbohra, chrisvanpatten, ellatrix and jorgefilipecosta as code owners Mar 30, 2020

youknowriad self-assigned this Mar 30, 2020

youknowriad force-pushed the update/toolbar-button-styles branch from 9852031 to cbdbf54 Compare Mar 30, 2020

youknowriad added 6 commits Apr 1, 2020

Add a generic style to the toolbar buttons

fb00904

more consistent toolbars

055e32b

Opinionated styles for all toolbars

5fd6b05

Fix breadcrumb

4fee8f1

Fix styles and round corners

0f4718b

Fix unit tests

9642fb3

jasmussen force-pushed the update/toolbar-button-styles branch from 6b99f2e to 9642fb3 Compare Apr 1, 2020

youknowriad merged commit 559655b into master Apr 2, 2020
3 checks passed

youknowriad deleted the update/toolbar-button-styles branch Apr 2, 2020

github-actions bot added this to the Gutenberg 7.9 milestone Apr 2, 2020

ZebulanStanphill mentioned this pull request Apr 3, 2020

Heading block: use toolbar for heading level control #20246

Merged

6 tasks

obenland reviewed Apr 20, 2020

View changes

earnjam mentioned this pull request May 1, 2020

Toolbars don't show selected item in sidebar #21022

Closed

earnjam mentioned this pull request May 1, 2020

Fix heading block sidebar level is-pressed styling #21064

Closed

youknowriad mentioned this pull request Jun 8, 2020

Image Block: add caption field to placeholder #22295

Merged

6 tasks

ntsekouras mentioned this pull request Jun 29, 2020

On nested Heading blocks, the "Change heading level" button loses padding when clicked #22683

Closed

WordPress / gutenberg Public

Add a generic style to the toolbar buttons #21252

Add a generic style to the toolbar buttons #21252

youknowriad commented Mar 30, 2020

github-actions bot commented Mar 30, 2020 •

edited

jasmussen commented Mar 30, 2020

youknowriad commented Mar 30, 2020

jasmussen commented Mar 30, 2020

youknowriad commented Mar 30, 2020

jasmussen commented Mar 30, 2020

youknowriad commented Mar 30, 2020

jasmussen commented Mar 30, 2020

ZebulanStanphill commented Mar 30, 2020

youknowriad commented Mar 31, 2020

youknowriad commented Apr 1, 2020

jasmussen commented Apr 1, 2020

jasmussen commented Apr 1, 2020

youknowriad commented Apr 1, 2020

youknowriad commented Apr 2, 2020 •

edited

jasmussen commented Apr 2, 2020

obenland Apr 20, 2020 •

edited

youknowriad Apr 21, 2020

youknowriad Apr 21, 2020

obenland Apr 21, 2020

youknowriad Apr 21, 2020

RickorDD commented Jun 7, 2020

youknowriad commented Jun 8, 2020

RickorDD commented Jun 8, 2020

WordPress / gutenberg Public

Add a generic style to the toolbar buttons #21252

Add a generic style to the toolbar buttons #21252

Conversation

youknowriad commented Mar 30, 2020

github-actions bot commented Mar 30, 2020 • edited

jasmussen commented Mar 30, 2020

youknowriad commented Mar 30, 2020

jasmussen commented Mar 30, 2020

youknowriad commented Mar 30, 2020

jasmussen commented Mar 30, 2020

youknowriad commented Mar 30, 2020

jasmussen commented Mar 30, 2020

ZebulanStanphill commented Mar 30, 2020

youknowriad commented Mar 31, 2020

youknowriad commented Apr 1, 2020

jasmussen commented Apr 1, 2020

jasmussen commented Apr 1, 2020

youknowriad commented Apr 1, 2020

youknowriad commented Apr 2, 2020 • edited

jasmussen commented Apr 2, 2020

obenland Apr 20, 2020 • edited

Choose a reason for hiding this comment

youknowriad Apr 21, 2020

Choose a reason for hiding this comment

youknowriad Apr 21, 2020

Choose a reason for hiding this comment

obenland Apr 21, 2020

Choose a reason for hiding this comment

youknowriad Apr 21, 2020

Choose a reason for hiding this comment

RickorDD commented Jun 7, 2020

youknowriad commented Jun 8, 2020

RickorDD commented Jun 8, 2020

github-actions bot commented Mar 30, 2020 •

edited

youknowriad commented Apr 2, 2020 •

edited

obenland Apr 20, 2020 •

edited