WIP: [Block Library - Social Icons]: Make the block use the new flex layout#33987
Closed
ntsekouras wants to merge 2 commits intotrunkfrom
Closed
WIP: [Block Library - Social Icons]: Make the block use the new flex layout#33987ntsekouras wants to merge 2 commits intotrunkfrom
flex layout#33987ntsekouras wants to merge 2 commits intotrunkfrom
Conversation
ntsekouras
added
[Block] Social
ntsekouras
requested review from
aristath,
gziolo,
jorgefilipecosta,
oandregal and
youknowriad
|
Size Change: -833 B (0%) Total Size: 1.03 MB
ℹ️ View Unchanged
|
youknowriad
reviewed
Aug 11, 2021
Contributor
youknowriad
left a comment
youknowriad
left a comment
There was a problem hiding this comment.
The approach is looking good to me overall, Left some remarks.
| const defaultLayout = useSetting( 'layout' ); | ||
| const { layout } = attributes; | ||
| // TODO: check if a theme should provide default values per `layoutType`. | ||
| // Fow now we use the values from `flow` (content, width). |
Contributor
There was a problem hiding this comment.
Not sure what you mean here?
Contributor
Author
There was a problem hiding this comment.
For now the default layout value in theme.json has not a type prop. It assumes it is the default/flow layout and this is how it is used for example in Group block. With the expansion of new layouts wouldn't we want themes to provide some defaults for other layouts as well?
Contributor
There was a problem hiding this comment.
In my mind, the theme provides a single default layout and chooses its type and doesn't provide defaults for all layouts. The default layout is automatically used in the post editor canvas.
Contributor
Author
There was a problem hiding this comment.
So the provided by the theme default should be only about flow layout, no?
Contributor
There was a problem hiding this comment.
maybe a theme would want to use a "grid" layout by default or some other layout we didn't build yet but yeah in most cases, it will be a "flow" one I think.
| [ clientId ] | ||
| ); | ||
|
|
||
| const usedLayout = !! layout || getDefaultBlockLayout( name ); |
Contributor
There was a problem hiding this 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.
| $style .= "justify-content: $justify_content;"; | ||
| $style .= '}'; | ||
| } | ||
|
|
Contributor
There was a problem hiding this 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.
3 tasks
youknowriad
reviewed
Aug 12, 2021
|
|
||
| const onChangeType = ( newType ) => | ||
| setAttributes( { layout: { type: newType } } ); | ||
| setAttributes( { layout: { type: newType } } ); // TODO needs checking with block defaults. |
Contributor
There was a problem hiding this comment.
Can you clarify this comment?
Contributor
Author
There was a problem hiding this comment.
I'm not sure yet 😄 . I'm adding some comments to check it more when the time comes. I just thought I should check if we would need to add the defaultBlockLayout props if exist as well, because this line will trigger a rerender and the logic is if layout is truthy. Therefore I'm not sure it will behave properly.
7 tasks
7 tasks
Contributor
|
Looks like we already landed a few things from this PR, should we try to refresh it maybe? Is this still on your radar? |
Contributor
Author
Yes it is. I'm catching up with stuff and I plan on working on this soon. |
Contributor
|
Per a recent nav block refactor to use gap (#32367), I'd love to take it a step further and leverage the new layout controls and theme.json provided gap definitions. So if you need any of my help on this one, I'd be happy to! |
Contributor
Author
|
I've created a new PR for this here: #33987. I'll add to the new PR a couple of comments that were not answered to continue the discussions there. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Related to: #33359
This PR is in very early stage, super rough 😄 and tries to explore the new
flex layoutto be used bySocial Iconsblock. These explorations will result in discussions about finding the best approach to utilizelayoutto existent blocks likeButtonsetc..I'll update the PR's description with my observations and questions. For now I've some comments in the code, for early reviewers.
Notes
Social Iconsblock and I'll create a separate issue for it, regardingitemsJustificationattribute which was introduced in Add Items Justification to Social Links #28980 and wasn't declared in block.json.trunkif you select the block, the alignment is different than the one applied at the end (it usesflex-start.Early testing instructions
Social Iconsblock and in Inspector controls play with the justify content control which has icons for now 😆