ui-kit icon indicating copy to clipboard operation
ui-kit copied to clipboard

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

Open ahmehri opened this issue 4 years ago • 8 comments

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 avatar Dec 10 '21 18:12 ahmehri

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.

ahmehri avatar Dec 10 '21 19:12 ahmehri

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

emmenko avatar Dec 22 '21 13:12 emmenko

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)

mmaltsev-ct avatar Feb 07 '22 09:02 mmaltsev-ct

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.

kark avatar Feb 17 '22 13:02 kark

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.

ahmehri avatar Mar 02 '22 15:03 ahmehri

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:

  • there might be cases where it might not be preferable (e.g. vertically stacked layouts mentioned before) The end effect would always be like: Screenshot 2022-02-17 at 14 43 54 while we might want: Screenshot 2022-02-17 at 14 40 22

  • we now have multiple ways of doing that in the client code :tada:

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

kark avatar Mar 04 '22 15:03 kark

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.

ahmehri avatar Mar 04 '22 16:03 ahmehri

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

kark avatar Mar 09 '22 14:03 kark