Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix HStack and VStack alignment prop #47914

Merged
merged 7 commits into from
Mar 2, 2023
Merged

Fix HStack and VStack alignment prop #47914

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
754d5b7
Fix HStack and VStack alignment prop
youknowriad Feb 9, 2023
aa32c1e
Better default values
youknowriad Feb 9, 2023
172ee45
Fix unit tests
youknowriad Feb 9, 2023
08722d9
Fix typo
youknowriad Feb 28, 2023
836e38d
VStack: Fix types
mirka Feb 28, 2023
986f6c1
Fix documentation
youknowriad Feb 28, 2023
9704e62
Fix unit tests
youknowriad Feb 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 16 additions & 14 deletions packages/block-editor/src/components/color-palette/test/__snapshots__/control.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,17 +22,18 @@ exports[`ColorPaletteControl matches the snapshot 1`] = `
display: -webkit-flex;
display: -ms-flexbox;
display: flex;
-webkit-align-items: normal;
-webkit-box-align: normal;
-ms-flex-align: normal;
align-items: normal;
-webkit-align-items: strech;
-webkit-box-align: strech;
-ms-flex-align: strech;
align-items: strech;
-webkit-flex-direction: column;
-ms-flex-direction: column;
flex-direction: column;
gap: calc(4px * 1);
-webkit-box-pack: justify;
-webkit-justify-content: space-between;
justify-content: space-between;
-webkit-box-pack: center;
-ms-flex-pack: center;
-webkit-justify-content: center;
justify-content: center;
}

.emotion-4>* {
Expand All @@ -54,17 +55,18 @@ exports[`ColorPaletteControl matches the snapshot 1`] = `
display: -webkit-flex;
display: -ms-flexbox;
display: flex;
-webkit-align-items: normal;
-webkit-box-align: normal;
-ms-flex-align: normal;
align-items: normal;
-webkit-align-items: strech;
-webkit-box-align: strech;
-ms-flex-align: strech;
align-items: strech;
-webkit-flex-direction: column;
-ms-flex-direction: column;
flex-direction: column;
gap: calc(4px * 3);
-webkit-box-pack: justify;
-webkit-justify-content: space-between;
justify-content: space-between;
-webkit-box-pack: center;
-ms-flex-pack: center;
-webkit-justify-content: center;
justify-content: center;
}

.emotion-8>* {
Expand Down
30 changes: 16 additions & 14 deletions packages/components/src/color-palette/test/__snapshots__/index.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,17 +6,18 @@ exports[`ColorPalette should allow disabling custom color picker 1`] = `
display: -webkit-flex;
display: -ms-flexbox;
display: flex;
-webkit-align-items: normal;
-webkit-box-align: normal;
-ms-flex-align: normal;
align-items: normal;
-webkit-align-items: strech;
-webkit-box-align: strech;
-ms-flex-align: strech;
align-items: strech;
-webkit-flex-direction: column;
-ms-flex-direction: column;
flex-direction: column;
gap: calc(4px * 3);
-webkit-box-pack: justify;
-webkit-justify-content: space-between;
justify-content: space-between;
-webkit-box-pack: center;
-ms-flex-pack: center;
-webkit-justify-content: center;
justify-content: center;
}

.emotion-0>* {
Expand Down Expand Up @@ -103,17 +104,18 @@ exports[`ColorPalette should render a dynamic toolbar of colors 1`] = `
display: -webkit-flex;
display: -ms-flexbox;
display: flex;
-webkit-align-items: normal;
-webkit-box-align: normal;
-ms-flex-align: normal;
align-items: normal;
-webkit-align-items: strech;
-webkit-box-align: strech;
-ms-flex-align: strech;
align-items: strech;
-webkit-flex-direction: column;
-ms-flex-direction: column;
flex-direction: column;
gap: calc(4px * 3);
-webkit-box-pack: justify;
-webkit-justify-content: space-between;
justify-content: space-between;
-webkit-box-pack: center;
-ms-flex-pack: center;
-webkit-justify-content: center;
justify-content: center;
}

.emotion-0>* {
Expand Down
4 changes: 2 additions & 2 deletions packages/components/src/flex/flex/hook.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ function useDeprecatedProps(

export function useFlex( props: WordPressComponentProps< FlexProps, 'div' > ) {
const {
align = 'center',
align,
className,
direction: directionProp = 'row',
expanded = true,
Expand All @@ -62,7 +62,7 @@ export function useFlex( props: WordPressComponentProps< FlexProps, 'div' > ) {

const classes = useMemo( () => {
const base = css( {
alignItems: isColumn ? 'normal' : align,
alignItems: align ?? ( isColumn ? 'normal' : 'center' ),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change definitely makes sense, but I'm not sure if the new behaviour of HStack is 100% correct either.

I was taking a look at HStack when direction="column" , and I was playing around with the alignment prop:

  • on trunk, in this case changing the alignment prop doesn't seem to make any differences
  • on this PR, it seems to work as expected, except for the alignment="edge" case - the items appear to be centered, but I believed they should be stretched (especially given the description of the edge value: "Aligns content to the edges of the container.")
Looks like this change would fix it 
diff --git a/packages/components/src/h-stack/utils.ts b/packages/components/src/h-stack/utils.ts
index b1413ee2dc..60151b00a2 100644
--- a/packages/components/src/h-stack/utils.ts
+++ b/packages/components/src/h-stack/utils.ts
@@ -28,7 +28,7 @@ const V_ALIGNMENTS: Alignments = {
 	bottomLeft: { justify: 'flex-end', align: 'flex-start' },
 	bottomRight: { justify: 'flex-end', align: 'flex-end' },
 	center: { justify: 'center', align: 'center' },
-	edge: { justify: 'space-between', align: 'center' },
+	edge: { justify: 'space-between', align: 'stretch' },
 	left: { justify: 'center', align: 'flex-start' },
 	right: { justify: 'center', align: 'flex-end' },
 	stretch: { align: 'stretch' },

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I remember properly, I based the behavior of "edge" here by comparing the behavior of "h" in HStack and applied the same behavior but in the other axis.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As much as I don't love the arbitrariness of this align prop, I agree with Riad that this new edge behavior for VStack is correct/consistent within the existing logic of the system.

In this system, I understand stretch to apply only to the cross axis, and edge to apply only to the main axis.

How about we tweak the description to make that clearer?

-	 * * `edge`: Aligns content to the edges of the container.
+	 * * `edge`: Justifies content to be evenly spread out up to the main axis edges of the container.
-	 * * `stretch`: Stretches content to the edges of the container.
+	 * * `stretch`: Stretches content to the cross axis edges of the container.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you both.

Given that the separation between breaking change and bug fix can be blurry here, I agree with Lena's proposed solution

flexDirection: direction,
flexWrap: wrap ? 'wrap' : undefined,
gap: space( gap ),
Expand Down
6 changes: 3 additions & 3 deletions packages/components/src/h-stack/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import { isValueDefined } from '../utils/values';

const H_ALIGNMENTS: Alignments = {
bottom: { align: 'flex-end', justify: 'center' },
bottomLeft: { align: 'flex-start', justify: 'flex-end' },
bottomLeft: { align: 'flex-end', justify: 'flex-start' },
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was buggy (inverted values)

bottomRight: { align: 'flex-end', justify: 'flex-end' },
center: { align: 'center', justify: 'center' },
edge: { align: 'center', justify: 'space-between' },
Expand All @@ -25,13 +25,13 @@ const H_ALIGNMENTS: Alignments = {

const V_ALIGNMENTS: Alignments = {
bottom: { justify: 'flex-end', align: 'center' },
bottomLeft: { justify: 'flex-start', align: 'flex-end' },
bottomLeft: { justify: 'flex-end', align: 'flex-start' },
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was buggy (inverted values)

bottomRight: { justify: 'flex-end', align: 'flex-end' },
center: { justify: 'center', align: 'center' },
edge: { justify: 'space-between', align: 'center' },
left: { justify: 'center', align: 'flex-start' },
right: { justify: 'center', align: 'flex-end' },
stretch: { justify: 'stretch' },
stretch: { align: 'stretch' },
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was buggy too.

top: { justify: 'flex-start', align: 'center' },
topLeft: { justify: 'flex-start', align: 'flex-start' },
topRight: { justify: 'flex-start', align: 'flex-end' },
Expand Down
10 changes: 6 additions & 4 deletions packages/components/src/v-stack/hook.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,16 @@ import type { VStackProps } from './types';
export function useVStack(
props: WordPressComponentProps< VStackProps, 'div' >
) {
const { expanded = false, ...otherProps } = useContextSystem(
props,
'VStack'
);
const {
expanded = false,
alignment = 'strech',
youknowriad marked this conversation as resolved.
Show resolved Hide resolved
...otherProps
} = useContextSystem( props, 'VStack' );

const hStackProps = useHStack( {
direction: 'column',
expanded,
alignment,
...otherProps,
} );

Expand Down
35 changes: 28 additions & 7 deletions packages/components/src/v-stack/stories/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,29 @@ import type { ComponentMeta, ComponentStory } from '@storybook/react';
import { View } from '../../view';
import { VStack } from '..';

const ALIGNMENTS = {
top: 'top',
topLeft: 'topLeft',
topRight: 'topRight',
left: 'left',
center: 'center',
right: 'right',
bottom: 'bottom',
bottomLeft: 'bottomLeft',
bottomRight: 'bottomRight',
edge: 'edge',
stretch: 'stretch',
};

const meta: ComponentMeta< typeof VStack > = {
component: VStack,
title: 'Components (Experimental)/VStack',
argTypes: {
alignment: { control: { type: 'text' } },
alignment: {
control: { type: 'select' },
options: Object.keys( ALIGNMENTS ),
mapping: ALIGNMENTS,
},
as: { control: { type: 'text' } },
direction: { control: { type: 'text' } },
justify: { control: { type: 'text' } },
Expand All @@ -28,12 +46,15 @@ export default meta;

const Template: ComponentStory< typeof VStack > = ( props ) => {
return (
<VStack { ...props }>
<View>One</View>
<View>Two</View>
<View>Three</View>
<View>Four</View>
<View>Five</View>
<VStack
{ ...props }
style={ { background: '#eee', minHeight: '200px' } }
>
{ [ 'One', 'Two', 'Three', 'Four', 'Five' ].map( ( text ) => (
<View key={ text } style={ { background: '#b9f9ff' } }>
{ text }
</View>
) ) }
Comment on lines +49 to +57
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this improvement!

</VStack>
);
};
Expand Down
38 changes: 20 additions & 18 deletions packages/components/src/v-stack/test/__snapshots__/index.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,10 @@ exports[`props should render alignment 1`] = `
display: -webkit-flex;
display: -ms-flexbox;
display: flex;
-webkit-align-items: normal;
-webkit-box-align: normal;
-ms-flex-align: normal;
align-items: normal;
-webkit-align-items: center;
-webkit-box-align: center;
-ms-flex-align: center;
align-items: center;
-webkit-flex-direction: column;
-ms-flex-direction: column;
flex-direction: column;
Expand Down Expand Up @@ -46,17 +46,18 @@ exports[`props should render correctly 1`] = `
display: -webkit-flex;
display: -ms-flexbox;
display: flex;
-webkit-align-items: normal;
-webkit-box-align: normal;
-ms-flex-align: normal;
align-items: normal;
-webkit-align-items: strech;
-webkit-box-align: strech;
-ms-flex-align: strech;
align-items: strech;
-webkit-flex-direction: column;
-ms-flex-direction: column;
flex-direction: column;
gap: calc(4px * 2);
-webkit-box-pack: justify;
-webkit-justify-content: space-between;
justify-content: space-between;
-webkit-box-pack: center;
-ms-flex-pack: center;
-webkit-justify-content: center;
justify-content: center;
}

.emotion-0>* {
Expand Down Expand Up @@ -85,17 +86,18 @@ exports[`props should render spacing 1`] = `
display: -webkit-flex;
display: -ms-flexbox;
display: flex;
-webkit-align-items: normal;
-webkit-box-align: normal;
-ms-flex-align: normal;
align-items: normal;
-webkit-align-items: strech;
-webkit-box-align: strech;
-ms-flex-align: strech;
align-items: strech;
-webkit-flex-direction: column;
-ms-flex-direction: column;
flex-direction: column;
gap: calc(4px * 5);
-webkit-box-pack: justify;
-webkit-justify-content: space-between;
justify-content: space-between;
-webkit-box-pack: center;
-ms-flex-pack: center;
-webkit-justify-content: center;
justify-content: center;
}

.emotion-0>* {
Expand Down