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

BaseControl: Connect to context system #57408

Merged
merged 5 commits into from Jan 4, 2024
Merged

BaseControl: Connect to context system #57408

merged 5 commits into from Jan 4, 2024

Conversation

mirka
Copy link
Member

@mirka mirka commented Dec 27, 2023

Prerequisite for #56524

What?

Connects the BaseControl component to the context system.

Why?

These days we only connect components to the context system when we specifically need to, and there was a use case for it in #56524.

InputControl uses a BaseControl under the hood, which has __nextHasNoMarginBottom permanently set to true because InputControl never shipped with a margin bottom. However, we are in the process of reimplementing SearchControl with an InputControl, and SearchControl does in fact require an option for a backward compatible margin bottom.

Although we could theoretically add a "hidden" (hidden from docs) margin toggle prop to InputControl, that would be heavy-handed because this is likely the only time we're going to need it. I think contextConnect is the most generic and non-intrusive way to solve this.

Testing Instructions

In Storybook and in your IDE, smoke test the TS types for BaseControl, BaseControl.VisualLabel, and a few other context-connected components to see that they appear as expected.

@mirka mirka added [Type] Enhancement A suggestion for improvement. [Package] Components /packages/components labels Dec 27, 2023
@mirka mirka self-assigned this Dec 27, 2023
Copy link
Member Author

Choose a reason for hiding this comment

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

The changes in this file adds a new use case that was previously unsupported in the WordPressComponentProps type. It can now be used when the component does not pass through the rest props to an underlying element, i.e. when it does not blanket inherit types from an HTML element type. This can now be expressed as WordPressComponentProps< MyProps, null >.

Usually there is no reason to even use the WordPressComponentProps utility type in this case. But it turns out that the way contextConnect() infers its return type relies on the fact that the component prop types were created using WordPressComponentProps.

So I think this is currently the simplest way to get contextConnect() to return the correct component type.

@mirka mirka requested a review from a team December 27, 2023 21:19
return (
<Wrapper
className={ classnames( 'components-base-control', className ) }
Copy link
Member Author

Choose a reason for hiding this comment

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

Removed this because the context system actually adds the CSS class for us 😳

const classes = cx(
getStyledClassNameFromKey( namespace ),
props.className
);

Copy link
Contributor

@ciampo ciampo left a comment

Choose a reason for hiding this comment

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

🚀

@mirka mirka merged commit 1ba116b into trunk Jan 4, 2024
56 checks passed
@mirka mirka deleted the base-control-context branch January 4, 2024 12:39
@github-actions github-actions bot added this to the Gutenberg 17.5 milestone Jan 4, 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] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants