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

HStack, VStack: Stop passing invalid props #59416

Merged
merged 5 commits into from
Feb 28, 2024
Merged

Conversation

mirka
Copy link
Member

@mirka mirka commented Feb 27, 2024

Fixes a bug surfaced in #59193

What?

Stops the HStack and VStack components from passing through invalid props when a component is passed to the as prop.

Why?

The useHStack/useVStack hooks internally used the useFlex hook to generate props, which included the isColumn prop. This prop is actually used in the Flex component logic and not passed through to the underlying element, but was neither used nor omitted in the HStack/VStack components and thus passed through.

This is fine if an intrinsic element (such as "span") is used for the as prop, since the underlying View component (i.e. an Emotion-styled component) will correctly ignore non-intrinsic props. It can become a problem when a non-Emotion custom component is used for the as prop, because React will throw an error about the invalid prop.

How?

Omit the isColumn prop from the return props of the useHStack hook, which will also remove it from the useVStack hook.

Testing Instructions

  1. Check out 65b9624, which adds the unit tests for this bug. Tests should fail.
  2. After the commit with the fix, the tests should pass.

@mirka mirka added [Type] Bug An existing feature does not function as intended [Package] Components /packages/components labels Feb 27, 2024
@mirka mirka self-assigned this Feb 27, 2024
@mirka mirka requested a review from ajitbohra as a code owner February 27, 2024 17:15
Copy link

github-actions bot commented Feb 27, 2024

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: mirka <[email protected]>
Co-authored-by: aaronrobertshaw <[email protected]>
Co-authored-by: t-hamano <[email protected]>
Co-authored-by: tyxla <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@@ -22,7 +22,7 @@ import type { FlexProps } from '../types';

function useDeprecatedProps(
props: WordPressComponentProps< FlexProps, 'div' >
): WordPressComponentProps< FlexProps, 'div' > {
): Omit< typeof props, 'isReversed' > {
Copy link
Member Author

Choose a reason for hiding this comment

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

This return type was wrong because it wasn't omitting the deprecated isReversed prop, which is specifically being omitted in this hook. It wasn't causing a TS error per se, but it was making it look like useFlex could return another invalid prop (isReversed) in addition to isColumn.

Copy link
Contributor

@aaronrobertshaw aaronrobertshaw left a comment

Choose a reason for hiding this comment

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

Thanks for the fix and unit tests @mirka 👍

LGTM

Before After
Screenshot 2024-02-28 at 10 58 42 am Screenshot 2024-02-28 at 10 58 58 am

Copy link
Contributor

@t-hamano t-hamano left a comment

Choose a reason for hiding this comment

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

Thank you for the quick fix 🙏

I just want to ask you one thing: After this PR is merged, it's safe to use the implementation below, right?

<HStack as={ motion.div }></HStack>
<VStack as={ motion.div }></VStack>

Copy link
Member

@tyxla tyxla left a comment

Choose a reason for hiding this comment

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

Nice and straightforward, thanks! 🚀

@mirka
Copy link
Member Author

mirka commented Feb 28, 2024

After this PR is merged, it's safe to use the implementation below, right?

<HStack as={ motion.div }></HStack>
<VStack as={ motion.div }></VStack>

@t-hamano That's correct 👍

@mirka mirka enabled auto-merge (squash) February 28, 2024 13:34
@mirka mirka merged commit 9840fbe into trunk Feb 28, 2024
56 of 57 checks passed
@mirka mirka deleted the fix/h-stack-prop-error branch February 28, 2024 14:04
@github-actions github-actions bot added this to the Gutenberg 17.9 milestone Feb 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants