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

<Button> Width Style When Used In <Placeholder> #63206

Open
ObliviousHarmony opened this issue Jul 5, 2024 · 3 comments
Open

<Button> Width Style When Used In <Placeholder> #63206

ObliviousHarmony opened this issue Jul 5, 2024 · 3 comments
Labels
[Package] Components /packages/components [Type] Bug An existing feature does not function as intended

Comments

@ObliviousHarmony
Copy link
Contributor

Description

In woocommerce/woocommerce#48911 we found that #59275 introduced a new width style for all <Button> components used in a <Placeholder> component. This change here is a breaking change and we had to add a style to override it.

I would argue that the .components-placeholder__fieldset > * selector is also a bit aggressive, however, setting the width to all buttons used in a <Placeholder> component seems excessive.

Step-by-step reproduction instructions

  1. Add a <Button> component inside of a <Placeholder>
  2. Notice the width it sets on it.

Screenshots, screen recording, code snippet

No response

Environment info

No response

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

@ObliviousHarmony ObliviousHarmony added the [Type] Bug An existing feature does not function as intended label Jul 5, 2024
@t-hamano t-hamano added the [Package] Components /packages/components label Jul 6, 2024
@mirka
Copy link
Member

mirka commented Jul 8, 2024

I do agree this change was a bit aggressive and potentially breaking for existing third-party Placeholder consumers.

I understand the intent of this change was to responsively collapse elements when the container width is narrow. I can suggest, for example, adding something like a Placeholder.ResponsiveContainer subcomponent so consumers who want this behavior for some parts of their Placeholder UI can opt into it:

<Placeholder>
  <InputControl />
  <Placeholder.ResponsiveContainer>
    <Button>Primary</Button>
    <Button>Secondary</Button>
  </Placeholder.ResponsiveContainer>
</Placeholder>

@jasmussen Would something like that be an acceptable solution for the original intent?

@jasmussen
Copy link
Contributor

I understand the intent of this change was to responsively collapse elements when the container width is narrow.

That's correct, and suggestion sounds good at a glance.

@ObliviousHarmony
Copy link
Contributor Author

That seems like a really good way to support more aggressive responsiveness while still giving consumers plenty of control. We're using the useResizeObserver() hook to render an entirely different component at smaller breakpoints to handle responsiveness and so ideally the placeholder component isn't providing any responsiveness for us.

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

No branches or pull requests

4 participants