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

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

Open
ahmehri opened this issue Dec 10, 2021 · 8 comments
Labels
👶 good first contribution Good for newcomers 🐛 Type: Bug Something isn't working

Comments

@ahmehri
Copy link
Member

ahmehri commented Dec 10, 2021

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 ahmehri added the 🐛 Type: Bug Something isn't working label Dec 10, 2021
@ahmehri ahmehri changed the title Spacings.Inline: elements don't get stretched over the full container's width CollapsiblePanel: panel's content doesn't grow over the full container's width Dec 10, 2021
@ahmehri
Copy link
Member Author

ahmehri commented Dec 10, 2021

The panel's content is a Flex container:

const SectionContent = styled.div`
width: 100%;
display: flex;
flex-wrap: wrap;
align-items: flex-start;
`;

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
Copy link
Member

emmenko commented Dec 22, 2021

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

@mmaltsev-ct mmaltsev-ct added the 👶 good first contribution Good for newcomers label Feb 7, 2022
@mmaltsev-ct
Copy link
Contributor

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
Copy link
Contributor

kark commented Feb 17, 2022

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
Copy link
Member Author

ahmehri commented Mar 2, 2022

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 #2048 (comment). 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
Copy link
Contributor

kark commented Mar 4, 2022

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 🎉

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

@ahmehri
Copy link
Member Author

ahmehri commented Mar 4, 2022

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
Copy link
Contributor

kark commented Mar 9, 2022

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

@ddouglasz ddouglasz assigned ddouglasz and unassigned ddouglasz Oct 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
👶 good first contribution Good for newcomers 🐛 Type: Bug Something isn't working
Projects
None yet
Development

No branches or pull requests

5 participants