[Block Library - Social Links]: Use the new flex layout
#34493
Conversation
ntsekouras
added
[Block] Social
ntsekouras
requested review from
youknowriad,
gziolo,
kjellr,
jasmussen,
aaronrobertshaw and
a team
ntsekouras
requested review from
ellatrix,
mkaz,
spacedmonkey and
TimothyBJacobs
as code owners
| */ | ||
| if ( ! empty( $layout['justifyContent'] ) ) { | ||
| $style .= "justify-content: {$layout['justifyContent']};"; | ||
| } |
There was a problem hiding this comment.
@youknowriad 's comment:
I was expecting more changes here to retrieve the "default layout" from the block support config and apply it when the "layout" attribute is empty.
The passed layout has already checked the existence of default block layout.
| [ clientId ] | ||
| ); | ||
|
|
||
| const usedLayout = !! layout || getDefaultBlockLayout( name ); |
There was a problem hiding this comment.
@youknowriad 's comment:
All this logic is something that I wanted to do automatically in the hook for existing blocks using "layout", the problem is that we don't have a filter for InnerBlocks to do it. Maybe we should introduce an unstable filter to be able to inject the layout prop into InnerBlocks.
There was a problem hiding this comment.
I'll explore this..
There was a problem hiding this comment.
@ntsekouras this is separate from this PR, don't make it block you here.
|
Size Change: +2.41 kB (0%) Total Size: 1.04 MB
ℹ️ View Unchanged
|
|
This is excellent:
Note the above has a pretty spacious (intentionally so to test) margin, but it's applied correctly:
Here's the gap highlighted by the inspector:
Since we are employing gap now, I think we can remove the margin that sits around each item, which was previously there just to space things out:
I did notice in the resting state, this "click plus" prompt is subject to the issue as well:
I never really loved that help text in the first place, I think we'll want to do something different. But a small interim fix we can push in the mean time might just be to set the gap to zero when in that setup state. Another thing I noticed, which we also don't have to address here, is that the Button component which is intended only for use inside editor UI is used for the markup, when an actual
I also tried customizing the gap for this block in particular, so that it has its own rules and not those inherited from the block gap defined in works as intended:
— though it does show we still have to remove the margins from the individual items still. It also opens another question: should we provide a block gap rule just for this block, or do we want it to be inherited from the default 24px styles rule? I don't have a strong opinion, but my instinct is that people would want large-ish block gaps for the main content, and a somewhat smaller gap inside social links. |
Can't we wrap the plus and the text in a single element? |
I agree with this but it means we might need a |
That is a good though but also one that relates more to how block gap works and not specific to this PR. It seems like you're suggesting one of the two 1- For simplicity, I personally prefer the current behavior (consistent blockGap) but if we do want to make the distinction, maybe the first option is better. It also related to a previous feedback by @andrewserong : Do we need both "column gap" and "row gap" which also raises the question about whether "row gap" in a "flow" container is equivalent to a "row gap" in a "flex" container |
The search block was recently refactored along the same lines: #33961 — if mobile is an issue, we need to look back to that one as well.
I think maybe I'm suggesting a 3rd option, one where there's a global CSS variable that is inherited, and then a default definition in the bundled lib/theme.json that overrides it. Someting like: It's not a fully formed thought — mostly just sharing the thought process at this point. |
That works for me 👍 |
This seems to only work in block-based themes. I'm generally using non-block based themes for working on the widgets screen, and noticed justification options are no longer available. It might be on your radar, but thought I'd mention it. |
|
Thanks for the ping!
That should now work now that #33991 has landed. Adding the block gap value for a specific block in the |
ntsekouras
force-pushed
the
add/flex-justify-content-control
branch
from
91e594b
to
72a05ee
Compare
Thanks for testing @talldan! I've made some changes to work on all themes. Also |
test/integration/fixtures/blocks/core__social-links.serialized.html
Outdated
Show resolved
Hide resolved
|
I pushed a small fix to remove the margin on link items, since they are now spaced apart wiht gap. The gap in the following is intentionally crazy to show that it works, but we'd want a sane default, like 1em, as suggested previously:
The gap looks equally crazy in this case, as noted:
However I wasn't able to immediately figure out how to move that prompt next to the appender only in the empty state. The CSS is a bit gnarly there. Since the gap will make sense in most cases, this might not be worth fixing since we are thinking of other options. I noted the comments about justification changes, but I'm not sure if they have UI consequences. In this branch I'm seeing justification tools moved to the inspector, like so:
The justification tools as part of the toolbar work better, in my opinion. Can we move them back to how they are in trunk?
|
ntsekouras
force-pushed
the
add/flex-justify-content-control
branch
from
7c7315c
to
df4f7b4
Compare
| } | ||
| return ( | ||
| <ToggleGroupControl | ||
| label={ __( 'Justify content' ) } |
There was a problem hiding this comment.
The label seems too close to the buttons in this case,
Also clicking the label doesn't seem to focus the buttons like labels do in general in controls.
I believe these are probably independent of this PR but could use some improvements separately.
|
I didn't have time to test the latest version here before landing, but we'll want to follow up on the segmented control if that's still used — the "space between" wrapping on two lines means that widget shouldn't be used. We could potentially use an iconified version of the segmented control that uses the justification icons, and reads a bit like this one:
|
@ntsekouras I was under the impression that there was some additional handling of className attribute. Via the custom class name hooks or in the parser where custom class names are "fixed". My suggestion was that although the current code in the deprecation only stripped out the justification class it might have had this unintended side effect.
Before this PR, alignment classes on existing blocks weren't treated as custom CSS classes. After a block is migrated via the new deprecations, the alignment class is saved as a custom CSS class. I've tested this again by checking out the commit on trunk prior to the merging of this PR. I get the same behaviour consistently. Steps to reproduce
Demo of above stepsScreen.Recording.2021-09-08.at.11.34.34.am.mp4 |
|
Thank you all for the excellent work on this. It's so nice to see the block toolbar affordance for justification still there. Nice work.
One thing we should follow up on soon is the segmented control where "Space between" wraps on to two lines:
The segmented control should not be used when there isn't space for the text inside to fit on one line. Some alternatives is a dropdown, or perhaps as alluded to earlier, a small buttongroup with icons. It could look like this:
The above, if we like, could even be a segmented control under the hood since only one item at a time is selected. But the design it emulates is that of toolbar buttons. What do you think, @ntsekouras? |
@jasmussen I'll open a follow up really soon. |
|
Awesome. Would be happy to pair on it 👌 |
* trunk: (214 commits) Fix snackbar overflow on nav editor (#34661) Mobile - Allow disabling text and background color via theme.json (#34633) Fix disabled blocks logical error on Widgets screen (#34634) [Mobile] - Global styles - Add support to render font sizes and line height (#34144) ESLint Plugin: Update eslint jsdoc dependency (#34338) Scripts: Add CHANGELOG entry for `jest-dev-server` upgrade (#34657) Bump jest-dev-server to v5 (#34560) Refactor the `core-data` store to thunks (#28389) Only capture toolbars on parent Nav block when not in vertical mode (#34615) Update Changelog for 11.5.0-rc.1 Bump plugin version to 11.5.0-rc.1 Gallery block: Fix media placeholder height in site editor (#34629) Border Controls: Display color indicator and check selected color (#34467) Remove horizontal and vertical navigation block variations from inserter (#34614) AlignmentMatrixControl : Fix/update docs (#34624) Gap block support: force gap change to cause the block to re-render (fix Safari issue) (#34567) [Block Library - Social Links]: Use the new `flex` layout (#34493) [Mobile] Update the bottom sheet header (#34309) Group block: Add a row variation (#34535) Migrate entities.js to thunks (#34582) ...
| @@ -0,0 +1,7 @@ | |||
| <!-- wp:social-links {"customIconColor":"#ffffff","iconColorValue":"#ffffff","customIconBackgroundColor":"#3962e3","iconBackgroundColorValue":"#3962e3","className":"has-icon-color"} --> | |||
| <ul class="wp-block-social-links has-icon-color has-icon-background-color items-justified-space-between"> | |||
There was a problem hiding this comment.
How can this block have item-justified-* classname without having the itemsJustification attribute?
There was a problem hiding this comment.
I guess that attribute has never been registered (was broken)
There was a problem hiding this comment.
Exactly.
I've found a bug in
Social Iconsblock regardingitemsJustificationattribute which was introduced in #28980 and wasn't declared in block.json. With this PR the issue will be resolved as we remove its usage.
Description
Related to: #33359
Fixes: #34003
This PR explores the new
flex layoutto be used bySocial Linksblock. The goal is to useflexlayout in a declarative way and not rely on 'hackishcss custom solutions for blocks that aflex` layout is a good fit.Notes
Social Iconsblock regardingitemsJustificationattribute which was introduced in Add Items Justification to Social Links #28980 and wasn't declared in block.json. With this PR the issue will be resolved as we remove it's usage.trunkif you select the block, the alignment is different than the one applied at the end (it usesflex-start. This PR doesn't have this problem.justify contentcontrol will need polishing in regards tolabelsetc..gapis applied now due to this previous change: https://github.com/WordPress/gutenberg/pull/34493/files#diff-324697e41855298e2f2c74b078f174e0cbc9075cef736ce9c1e2c169bf64652eR70.Testing instructions
Social Linksblock and in Inspector controls play with the justify content control.Social Linksblocks are displayed as before in both editor and front-end.Tasks