commercetools / ui-kit

Component library 💅
https://uikit.commercetools.com
MIT License
145 stars 25 forks source link

CollapsiblePanel: panel's content doesn't grow over the full container's width #2048

Open ahmehri opened 2 years ago

ahmehri commented 2 years ago

Describe the bug CollapsiblePanel's content doesn't grow over the full container's width.

To Reproduce Steps to reproduce the behavior:

Check the following code:

import {
  Card,
  CollapsiblePanel, Spacings, TextField
} from "@commercetools-frontend/ui-kit";

<CollapsiblePanel
  header={
    <CollapsiblePanel.Header>Collapsible Panel Header</CollapsiblePanel.Header>
  }
>
  <Spacings.Inline>
    <Card type="flat">
      <TextField name="variant-sku" title="SKU" horizontalConstraint={11} />
    </Card>
    <Card type="flat">
      <TextField name="variant-key" title="Key" horizontalConstraint={11} />
    </Card>
  </Spacings.Inline>
</CollapsiblePanel>;

Expected behavior CollapsiblePanel's content should grow over the full container's width.

Screenshots

CleanShot 2021-12-10 at 19 19 19@2x CleanShot 2021-12-10 at 19 19 38@2x

ahmehri commented 2 years ago

The panel's content is a Flex container:

https://github.com/commercetools/ui-kit/blob/c2e53b724f81ce668ec977c42042043877565752/packages/components/collapsible-panel/src/collapsible-panel.styles.tsx#L86-L92

The content's items (Flex items) are not growing over the container's width because the default value for flex-grow is being used, which is 0.

I think it's reasonable to expect that the panel's content should grow over the full panel's width, which means that a potential fix for the bug is to start using the value 1 for flex-grow.

emmenko commented 2 years ago

Thanks for reporting. We'll take a look at this and see what we can do.

mmaltsev-ct commented 2 years ago

In order to decide regarding the implementation, we'll check the usage of this component in the MC (both in the code and in the UI)

kark commented 2 years ago

Thank you again for reporting this issue. I am wondering if a wrapper component used with your proposed solution might be an acceptable solution:

import styled from "@emotion/styled";

const Wrapper = styled.span`
  flex-grow: 1;
`;

<CollapsiblePanel
      header={
        <CollapsiblePanel.Header>
          Collapsible Panel Header
        </CollapsiblePanel.Header>
      }
    >
      <Wrapper>
        <Spacings.Inline>
          ...
        </Spacings.Inline>
      </Wrapper>
</CollapsiblePanel>

without changes to the CollapsiblePanel component imposing flex-grow e.g. in case of vertically stacked layouts that perhaps might not be preferable:

Screenshot 2022-02-17 at 14 40 22

It would be great to hear your opinion about it.

ahmehri commented 2 years ago

Sorry for the late reply.

That's already the workaround that we use (although by using the ui-kit Constraints.Horizontal component instead).

As I expressed in https://github.com/commercetools/ui-kit/issues/2048#issuecomment-991235998. I think it makes sense to expect that the panel's content should grow over the full panel's width. This will still work even for vertically stacked layouts.

kark commented 2 years ago

Thank you for your reply and sharing another solution. Let me share our train of thought on that. From the UX point of view CollapsiblePanel is a wrapper component rendering any node passed as a children prop as they are. It seems we should not impose how children are presented.

Again, please consider:

However, we will look upon adding an optional prop to CollapsiblePanel as an enhancement if we collect more requests like this.

ahmehri commented 2 years ago

Thanks, I think the issue is more about providing better defaults. I think the panel's content growing over its full width is a better default. With this default, having vertically stacked layouts is still possible as shown in https://codesandbox.io/s/commercetools-ui-kit-2048-5w86f6?file=/src/App.js.

kark commented 2 years ago

Many thanks again for your reply and providing the code sample. We will need more time for researching how the defaults would influence all the use cases