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.

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

Fix HStack and VStack alignment prop #47914

Merged
merged 7 commits into from
Mar 2, 2023
Merged
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
@@ -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: stretch;
-webkit-box-align: stretch;
-ms-flex-align: stretch;
align-items: stretch;
-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>* {
@@ -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: stretch;
-webkit-box-align: stretch;
-ms-flex-align: stretch;
align-items: stretch;
-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>* {
Original file line number Diff line number Diff line change
@@ -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: stretch;
-webkit-box-align: stretch;
-ms-flex-align: stretch;
align-items: stretch;
-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>* {
@@ -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: stretch;
-webkit-box-align: stretch;
-ms-flex-align: stretch;
align-items: stretch;
-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>* {
4 changes: 2 additions & 2 deletions packages/components/src/flex/flex/hook.ts
Original file line number Diff line number Diff line change
@@ -40,7 +40,7 @@ function useDeprecatedProps(

export function useFlex( props: WordPressComponentProps< FlexProps, 'div' > ) {
const {
align = 'center',
align,
className,
direction: directionProp = 'row',
expanded = true,
@@ -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 ),
4 changes: 2 additions & 2 deletions packages/components/src/h-stack/README.md
Original file line number Diff line number Diff line change
@@ -44,8 +44,8 @@ Determines how the child elements are aligned.
- `bottom`: Aligns content to the bottom.
- `bottomLeft`: Aligns content to the bottom/left.
- `bottomRight`: Aligns content to the bottom/right.
- `edge`: Aligns content to the edges of the container.
- `stretch`: Stretches 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 cross axis edges of the container.

##### direction

4 changes: 2 additions & 2 deletions packages/components/src/h-stack/types.ts
Original file line number Diff line number Diff line change
@@ -41,8 +41,8 @@ export type Props = Omit< FlexProps, 'align' | 'gap' > & {
* * `bottom`: Aligns content to the bottom.
* * `bottomLeft`: Aligns content to the bottom/left.
* * `bottomRight`: Aligns content to the bottom/right.
* * `edge`: Aligns content to the edges of the container.
* * `stretch`: Stretches 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 cross axis edges of the container.
*
* @default 'edge'
*/
6 changes: 3 additions & 3 deletions packages/components/src/h-stack/utils.ts
Original file line number Diff line number Diff line change
@@ -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' },
@@ -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' },
4 changes: 2 additions & 2 deletions packages/components/src/v-stack/README.md
Original file line number Diff line number Diff line change
@@ -42,8 +42,8 @@ Determines how the child elements are aligned.
- `bottom`: Aligns content to the bottom.
- `bottomLeft`: Aligns content to the bottom/left.
- `bottomRight`: Aligns content to the bottom/right.
- `edge`: Aligns content to the edges of the container.
- `stretch`: Stretches 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 cross axis edges of the container.

##### `direction`: `FlexDirection`

10 changes: 6 additions & 4 deletions packages/components/src/v-stack/hook.ts
Original file line number Diff line number Diff line change
@@ -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 = 'stretch',
...otherProps
} = useContextSystem( props, 'VStack' );

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

35 changes: 28 additions & 7 deletions packages/components/src/v-stack/stories/index.tsx
Original file line number Diff line number Diff line change
@@ -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' } },
@@ -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>
);
};
38 changes: 20 additions & 18 deletions packages/components/src/v-stack/test/__snapshots__/index.tsx.snap
Original file line number Diff line number Diff line change
@@ -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;
@@ -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: stretch;
-webkit-box-align: stretch;
-ms-flex-align: stretch;
align-items: stretch;
-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>* {
@@ -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: stretch;
-webkit-box-align: stretch;
-ms-flex-align: stretch;
align-items: stretch;
-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>* {
8 changes: 5 additions & 3 deletions packages/components/src/v-stack/types.ts
Original file line number Diff line number Diff line change
@@ -8,7 +8,7 @@ import type { CSSProperties } from 'react';
*/
import type { HStackAlignment, Props as HStackProps } from '../h-stack/types';

export type VStackProps = HStackProps & {
export type VStackProps = Omit< HStackProps, 'alignment' | 'spacing' > & {
/**
* Determines how the child elements are aligned.
*
@@ -21,8 +21,10 @@ export type VStackProps = HStackProps & {
* - `bottom`: Aligns content to the bottom.
* - `bottomLeft`: Aligns content to the bottom/left.
* - `bottomRight`: Aligns content to the bottom/right.
* - `edge`: Aligns content to the edges of the container.
* - `stretch`: Stretches 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 cross axis edges of the container.
*
* @default 'stretch'
*/
alignment?: HStackAlignment | CSSProperties[ 'alignItems' ];
/**