Adds in missing styling for toolbar when using text-only setting #26769
Conversation
| @@ -189,6 +189,10 @@ | |||
| } | |||
| } | |||
|
|
|||
| .edit-post-header-toolbar__left > * + * { | |||
There was a problem hiding this comment.
This is a really obtuse selector. What exactly does it do, and is there a CSS class you can target instead?
There was a problem hiding this comment.
It is, but there's a bit of a tangled web going on in that code. I'll loop @tellthemachines in here (she was great in suggesting this fix) as I do think there's room for iteration and maybe simplification here. Spoiler, there is always room for reducing!
There was a problem hiding this comment.
This is the (somewhat unfortunately named) lobotomized owl selector. It targets all sibling elements except the first ( * + * meaning literally "any element that comes after another element"). Useful here because by adding a left margin to all but the first element, we ensure there's no extra margin at either end of the toolbar. Though it's possible to do the same with classes (.classname + .classname, we have a few of those sprinkled around the codebase), in this case the children of edit-post-header-toolbar__left don't all have the same class, and are not even all the same element, so * is the cleanest way to target them all.
Perhaps a comment explaining the selector would be helpful here? Something like Select all but the first direct children of .edit-post-header-toolbar__left?
There was a problem hiding this comment.
Because this rule is specific to the toolbar, it should probably live inside packages/edit-post/src/components/header/header-toolbar/style.scss instead. Also, it needs to be scoped to the text-only option, because currently the margin is being added on the icon buttons too. So I reckon the best place to put it is inside the .show-icon-labels rule on line 143 of the header-toolbar file.
|
Size Change: +49 B (0%) Total Size: 1.21 MB
ℹ️ View Unchanged
|
|
I see this:
Which true to your screenshots, looks slightly better than before. Nice! I was about to say we need to verify this works well for the site editor, which uses a separate stylesheet, but it appears the text label option doesn't work there at all 🙈 I left a small comment, but otherwise this seems like a good small and neat PR. |
Yea, it should just be using/aligning with our tertiary button styles. And I believe those show a thicker border for focused elements than the hover state. Is that what's happening? In addition, I remember a discussion about dropping the tertiary style for buttons, but not sure it's been implemented yet. In which case, this becomes it's own style and not conforming to a supported design pattern which means anything goes 😉 . I could be completely off here though. |
| @@ -189,6 +189,10 @@ | |||
| } | |||
| } | |||
|
|
|||
| .edit-post-header-toolbar__left > * + * { | |||
There was a problem hiding this comment.
Because this rule is specific to the toolbar, it should probably live inside packages/edit-post/src/components/header/header-toolbar/style.scss instead. Also, it needs to be scoped to the text-only option, because currently the margin is being added on the icon buttons too. So I reckon the best place to put it is inside the .show-icon-labels rule on line 143 of the header-toolbar file.
|
@tellthemachines I have updated with those changes you requested. If there are any other please feel free to use the power of timezones to get us into beta, if that works. Hopefully it's now ready for a final review and send off. Thanks. |
faa30b6 to
9d51505
Compare
* Cover block: Restore default overlay background (#26569) * Restore default Cover overlay background The default Cover block overlay background was removed. This changed the style of existing blocks on existing sites. Restore the default background in such a way that it does not conflict with theme-provided background-color styles and there is no need to artificially add extra specificity. Fix regression: #26545 * Limit the interface skeleton to a max width of 100% to prevent some blocks managing to exceed the width (#26552) * Cover Block: Restore opacity controls (#26625) * Fix image block centering when resizing to a smaller size (#26376) * Fix image block centering when resizing to a smaller size * Revert previous 100% width fix * Remove display: table when image block is resized to avoid centering of block * Match frontend classes for alignment in editor * Gallery: Remove caption fallback for alt attribute (#26676) * Fix quote block default alignment (#26680) * Gallery: Remove obsolete deprecation entry (#26736) * Do not apply text color if it is being overriden (#24626) * Fix: Preset colors don't work on button block style outline (#26707) * Tests: Add fixture for Column deprecation (#26774) * Fix/column width units (#26757) * Fix issues with different units in column widths * Columns with fixed width should keep width on recalculation * Address review feedback * fix undefined index notice in Social Link Block (#25663) * fix undefined index notice * use isset instead of array_key_exists check * Update packages/block-library/src/social-link/index.php Co-authored-by: Greg Ziółkowski <grzegorz@gziolo.pl> Co-authored-by: Greg Ziółkowski <grzegorz@gziolo.pl> * Adds in missing styling for toolbar when using text-only setting (#26769) * Adds in missing styling for toolbar when using text-only setting * Increases margin * Moves style to correct file * Inserter: Fix 'Browse All' in Quick Inserter (#26443) * Inserter: Fix 'Browse All' in Quick Inserter Maintain the insertion point in @wordpress/block-editor state when moving from the Quick Inserter to the Inserter. This fixes a bug where the insertion point is wrong after clicking a block appender and selecting 'Browse All'. Care is taken to not regress a previous bug where the insertion point is wrong after clicking an inline insertion button and selecting 'Browse ALl'. * Inserter: Fix typo Co-authored-by: Daniel Richards <daniel.richards@automattic.com> * Call getBlockInsertionPoint once * Add useSelect dependencies * Make setInsertionPoint unstable * Avoid setting `isVisible` state when `SET_INSERTION_POINT` is triggered * Make index required and clarify rootClientId usage * Split insertionPoint into two reducers * Fix getInsertionPoint selector that expects default state of reducer to be null * Avoid resetting insertionPoint state on HIDE_INSERTION_POINT Co-authored-by: Daniel Richards <daniel.richards@automattic.com> Co-authored-by: Jon Surrell <jon.surrell@automattic.com> Co-authored-by: Jacopo Tomasone <Copons@users.noreply.github.com> Co-authored-by: Daniel Richards <daniel.richards@automattic.com> Co-authored-by: Bernie Reiter <ockham@raz.or.at> Co-authored-by: Greg Ziółkowski <grzegorz@gziolo.pl> Co-authored-by: Oliver Juhas <webmandesign@users.noreply.github.com> Co-authored-by: Jorge Costa <jorge.costa@automattic.com> Co-authored-by: Fabian Kägy <mail@fabian-kaegy.de> Co-authored-by: Tammie Lister <tammie@automattic.com> Co-authored-by: Robert Anderson <robert@noisysocks.com>
This is a fix for the missing styling from and closes #26748. Props to @mapk and @tellthemachines for the historical context after this was found yesterday. I also brought in some additional space on the 'add' button and converted everything to grid variables. In the thread it was stated as being 10px margins, I used the grid 8px.
Before:
After:
Potential extra clean-up
It seems the selected state for these have a larger height. I don't know if this is desired or a mistake so looping @jasmussen in also to double check there. @mapk you might also be able to shed some light but I couldn't see a decision on that in the issue and wanted to ensure this didn't get held up by something could happen in another PR to fix select states.
Next steps
@tellthemachines if this is possible to get into the next release that would be great, you hinted it might be in the issue. I would love a review as particularly on the CSS class over-riding for text-only, unsure if that's the most desireable way of doing this.