Fix HStack and VStack alignment prop by youknowriad · Pull Request #47914 · WordPress/gutenberg

youknowriad · 2023-02-09T09:52:30Z

What?

While working on #47827 I've noticed a bug in VStack component. The alignment prop was not working as expected. It turns out that there are several bugs in the alignment prop of both VStack and HStack, This PR fixes it.

Testing Instructions

1- Open the storybook of both VStack and HStack
2- Check that all the alignment options behave as expected.

youknowriad · 2023-02-09T09:53:06Z

packages/components/src/flex/flex/hook.ts

 	const classes = useMemo( () => {
 		const base = css( {
-			alignItems: isColumn ? 'normal' : align,
+			alignItems: align,


This is the most important change. I'm not sure I understand the reason for the check originally, I checked the original PR and it's not mentioned.

youknowriad · 2023-02-09T09:53:20Z

packages/components/src/h-stack/utils.ts

 const H_ALIGNMENTS: Alignments = {
 	bottom: { align: 'flex-end', justify: 'center' },
-	bottomLeft: { align: 'flex-start', justify: 'flex-end' },
+	bottomLeft: { align: 'flex-end', justify: 'flex-start' },


This was buggy (inverted values)

youknowriad · 2023-02-09T09:53:26Z

packages/components/src/h-stack/utils.ts

 const V_ALIGNMENTS: Alignments = {
 	bottom: { justify: 'flex-end', align: 'center' },
-	bottomLeft: { justify: 'flex-start', align: 'flex-end' },
+	bottomLeft: { justify: 'flex-end', align: 'flex-start' },


This was buggy (inverted values)

youknowriad · 2023-02-09T09:53:34Z

packages/components/src/h-stack/utils.ts

 	left: { justify: 'center', align: 'flex-start' },
 	right: { justify: 'center', align: 'flex-end' },
-	stretch: { justify: 'stretch' },
+	stretch: { align: 'stretch' },


This was buggy too.

github-actions · 2023-02-09T10:17:03Z

Flaky tests detected in 172ee45.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4265445081
📝 Reported issues:

[Flaky Test] dims the background image down by 50% when transformed from the Image block #47052 in specs/editor/blocks/cover.test.js

youknowriad · 2023-02-24T08:27:55Z

Any thoughts on this PR @ciampo

ciampo · 2023-02-24T10:18:16Z

It's in my review queue — I hope to get to it by EOD.

I've been working on implementing some VizReg tests specifically on HStack and VStack (#48260) in the meantime, which will help while reviewing this PR.

ciampo

Alright — I took a first look at the code changes! Since it's quite tricky to understand the full implications of these changes, I used the recently introduced Visual Regression tests to compare this PR against trunk.

Here's what I'm doing to test this PR with VizReg

Checkout trunk
Install latest version of dependencies: npm run distclean && npm ci
Install PlayWright: npx playwright install
Generate snapshots:
- In one terminal window, run Storybook for VizReg: npm run storybook:e2e:dev
- Once Storybook is up and running, generate Playwright snapshots: npm run test:e2e:storybook -- --update-snapshots
Checkout this PR
Compare snapshots:
- In one terminal window, run Storybook for VizReg: npm run storybook:e2e:dev
- Once Storybook is up and running, compare VizReg snapshots: npm run test:e2e:storybook

I also rebased on top of trunk to include the latest VizReg changes

packages/components/src/v-stack/hook.ts

ciampo · 2023-02-24T18:40:30Z

packages/components/src/flex/flex/hook.ts

 	const classes = useMemo( () => {
 		const base = css( {
-			alignItems: isColumn ? 'normal' : align,
+			alignItems: align ?? ( isColumn ? 'normal' : 'center' ),


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' },

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.

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.

Thank you both.

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

mirka · 2023-02-28T09:38:26Z

packages/components/src/v-stack/stories/index.tsx

+		<VStack
+			{ ...props }
+			style={ { background: '#eee', minHeight: '200px' } }
+		>
+			{ [ 'One', 'Two', 'Three', 'Four', 'Five' ].map( ( text ) => (
+				<View key={ text } style={ { background: '#b9f9ff' } }>
+					{ text }
+				</View>
+			) ) }


Thanks for this improvement!

mirka · 2023-02-28T10:06:46Z

packages/components/src/flex/flex/hook.ts

 	const classes = useMemo( () => {
 		const base = css( {
-			alignItems: isColumn ? 'normal' : align,
+			alignItems: align ?? ( isColumn ? 'normal' : 'center' ),


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.

packages/components/src/v-stack/hook.ts

Co-authored-by: Marco Ciampini <marco.ciampo@gmail.com>

mirka

Thanks! 🚢

youknowriad added [Type] Bug An existing feature does not function as intended [Feature] UI Components Impacts or related to the UI component system labels Feb 9, 2023

youknowriad requested review from ciampo, mirka and walbo February 9, 2023 09:52

youknowriad self-assigned this Feb 9, 2023

youknowriad requested a review from ajitbohra as a code owner February 9, 2023 09:52

youknowriad commented Feb 9, 2023

View reviewed changes

ciampo added the [Package] Components /packages/components label Feb 9, 2023

youknowriad requested a review from ellatrix as a code owner February 9, 2023 13:39

ciampo mentioned this pull request Feb 20, 2023

VizReg e2e tests: programmatically test all combinations of a given list of props/values #48260

Merged

youknowriad added 3 commits February 24, 2023 18:20

Fix HStack and VStack alignment prop

754d5b7

Better default values

aa32c1e

Fix unit tests

172ee45

ciampo reviewed Feb 24, 2023

View reviewed changes

ciampo force-pushed the fix/vstack-hstack-alignments branch from 02d893c to 172ee45 Compare February 24, 2023 19:03

mirka reviewed Feb 28, 2023

View reviewed changes

youknowriad and others added 4 commits February 28, 2023 12:03

Fix typo

08722d9

Co-authored-by: Marco Ciampini <marco.ciampo@gmail.com>

VStack: Fix types

836e38d

Fix documentation

986f6c1

Fix unit tests

9704e62

mirka approved these changes Mar 1, 2023

View reviewed changes

youknowriad merged commit 03ec631 into trunk Mar 2, 2023

youknowriad deleted the fix/vstack-hstack-alignments branch March 2, 2023 06:20

github-actions bot added this to the Gutenberg 15.4 milestone Mar 2, 2023

Conversation

youknowriad commented Feb 9, 2023

What?

Testing Instructions

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Feb 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

youknowriad commented Feb 24, 2023

Uh oh!

ciampo commented Feb 24, 2023

Uh oh!

ciampo left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mirka left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions bot commented Feb 9, 2023 •

edited

Loading

ciampo left a comment •

edited

Loading