Add vertical alignment to blocks using flex control #40013
Merged
Conversation
This file contains 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
|
Size Change: +2.83 kB (0%) Total Size: 1.22 MB
ℹ️ View Unchanged
|
mtias
added
the
[Feature] Design Tools
youknowriad
reviewed
Apr 4, 2022
|
Thanks for the update, this looking better code wise, let's try to get some reviews on the design/behavior aspect. |
|
Doesn't seem to be working for me on a row block using the theme Remote (https://wordpress.org/themes/remote/)
|
@youknowriad there are still some adjustments to do in the |
|
Nice, thanks for the PR, took a quick spin playing around with the vertical toolbar alignment controls: High level, this will be a very useful feature to have. It also feels worthwhile to have a button present in the block toolbar as it's valuable. As shown in the GIF, I couldn't get the alignment to actually work — I tested a paragraph and an imagea, and the paragraph kept vertically centered. Did I do anything wrong? Some thoughts:
Finally, the Layout panel likely showing horizontal alignment but not vertical stands out as a little uncanny. It would be nice to find a design that could accommodate both of these. This is an area where Figma might be on to something, as it unifies both horizontal and vertical alignments into a single unified panel: This is not unlike what we already have for the Cover block:
In other words, there's an opportunity there to simplify. The larger question is whether that needs to block this PR. Probably not? |
|
@jasmussen @MaggieCabrera with d6c9253 you should be able to make it works. |
ntsekouras
reviewed
Apr 5, 2022
| @@ -113,9 +119,11 @@ export default { | |||
| const flexWrap = flexWrapOptions.includes( layout.flexWrap ) | |||
| ? layout.flexWrap | |||
| : 'wrap'; | |||
| const verticalAlignment = | |||
| verticalAlignmentMap[ layout.verticalAlignment ]; | |||
There was a problem hiding this comment.
You need to add a fallback to previous center value.
There was a problem hiding this comment.
Also the respective default for vertical orientation was the browser default so there was no visual issue, but in this case the center is not the default and we need to make the button active probably.
There was a problem hiding this comment.
Also the respective default for vertical orientation was the browser default so there was no visual issue, but in this
case the center is not the default and we need to make the button active probably.
I didn't really understand this part, could you please explain me in detail?
There was a problem hiding this comment.
It seems we just need this change. Disregard my mention to browser defaults, because code might have changed here 😄 .
The point was that the controls should match the default/fallback value you added.
aristath
reviewed
Apr 5, 2022
ntsekouras
reviewed
Apr 5, 2022
Co-authored-by: Nik Tsekouras <ntsekouras@outlook.com>
aristath
approved these changes
Apr 6, 2022
There was a problem hiding this comment.
Great work @JustinyAhin, thank you for working on this one! ❤️ 🎉
The code looks good and makes sense.
Tested it and works as advertised. 👍
mikachan
reviewed
Apr 7, 2022
| value: 'flex-end', | ||
| label: __( 'Align items bottom' ), | ||
| }, | ||
| ]; |
There was a problem hiding this comment.
I think these options need to be replicated in lib/block-supports/layout.php, similar to how the $justify_content_options array works. For example, by adding a vertical alignment options array:
$vertical_alignment_options = array(
'top' => 'flex-start',
'center' => 'center',
'bottom' => 'flex-end'
);
And then using these options, with $layout['verticalAlignment'], around line 111, inside the $layout_orientation check:
if ( ! empty( $layout['verticalAlignment'] ) && array_key_exists( $layout['verticalAlignment'], $vertical_alignment_options ) ) {
$style .= "align-items: {$vertical_alignment_options[ $layout['verticalAlignment'] ]};";
} else {
$style .= 'align-items: center;';
}
JustinyAhin
requested review from
TimothyBJacobs and
spacedmonkey
as code owners
mikachan
approved these changes
Apr 7, 2022
JustinyAhin
changed the title
|
What is the expected result for the navigation block which uses the flex control? I would like to be able to align the text items with the top of the logo, for example. https://user-images.githubusercontent.com/7422055/162396948-9551db45-e9c6-4a21-a668-bf01ae7a4ada.mp4 |
|
Buttons work well: https://user-images.githubusercontent.com/7422055/162398248-1e95c97b-d6cf-4def-bfee-f8235f6ed40d.mp4 |
|
Query pagination, comment query pagination -Works in the editor and front. |
|
I added a condition that will only render the control when using the horizontal alignment, and implemented a new |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fixes #38498
What?
This PR adds a new block control for vertical alignment to the blocks that use flex control.
Why?
See #38498
How?
This adds a new
verticalAlignment(gutenberg/packages/block-editor/src/components/block-vertical-alignment-control/index.js
Line 17 in 190a207
align-itemsstyles.Testing Instructions
heightproperty from the browser inspector)