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

[Feature][Collapsible] - Allow uncontrolled behavior #51

Open
yairEO opened this issue Jun 3, 2021 · 4 comments
Open

[Feature][Collapsible] - Allow uncontrolled behavior #51

yairEO opened this issue Jun 3, 2021 · 4 comments
Labels
Collapsible enhancement New feature or request

Comments

@yairEO
Copy link
Contributor

yairEO commented Jun 3, 2021

Currently the <Collapsible/> may only work as a controlled component by using the expanded prop.

Sometimes there is no need for outside control and it is wished for the component to show/hide content when clicked, so another prop can be introduced: defaultExpanded for the initial state, and from there the component will handle the state internally, as long as the expanded prop is not defined.

Also, this requires another sub-component, which I suggest something like Collapsible.Trigger which will act as the "clicking" area that shows/hides content, this will also give extra control where the triggering content is rendered relative to the collapsible content.

<Collapsible defaultExpanded={true} id='foo' onClose={...}>
    Lorem ipsum dolor sit amet
</Collapsible>
...
<Collapsible.Trigger for='foo'>Toggle me</Collapsible.Trigger>

Alternatively, add ability to place the <Trigger> inside:

<Collapsible>
    <Collapsible.Trigger>Toggle me</Collapsible.Trigger>
    Lorem ipsum dolor sit amet
</Collapsible>

Then it will be possible to clone & place it outside of the actual collapsible wrapper, so it will always be visible.


This will free developers from creating their own states which is especially time consuming when wanting to use a large number of Collapsible components

@ykadosh
Copy link
Collaborator

ykadosh commented Jun 4, 2021

Having a component that is both controlled and uncontrolled can lead to many issues, and can become a maintenance nightmare. This is discouraged in the official React docs (they refer to it as a "mistake" no less) and I've also written an article about why it should be avoided - Don’t Mix Controlled and Uncontrolled Values In React.

Moreover, if we decide to take it upon ourselves to provide this functionality out of the box, we will have to consider every possible behavior that the user may want to have. For example, what if the consumer would like the trigger to take place on hover instead of click? What if he wants it to only show or only hide, instead of toggle?

That said, I don't think there's much to gain from it anyway. Instead of adding 2 new props (defaultExpanded, onClose) and a new component (<Collapsible.Trigger/>), and all the internal logic that comes with that, the same thing can be done by adding a single line of code to the containing layer:

const {visible, toggle} = useVisibilityState();

<div onClick={toggle}>Toggle</div>

<Collapsible expanded={visible}>
    Lorem ipsum dolor sit amet
</Collapsible>

As for having a large number of collapsibles, the above code can be turned into a component and used as many times as needed.

@yairEO
Copy link
Contributor Author

yairEO commented Jun 4, 2021

I wanted to use Collapsible in some story in Storybook which is written in MDX format, and I wanted to place there lots of collapsible components without managing any state on an MDX file. State makes managing MDX stories more difficult.

Normally I use:

<details>
	<summary>click to toggle</summary>
	content here
</details>

But it's not as visually pleasing as the Collapsible 🙂

Without having it manage its own open/close state I don't think MDX use-case is very practical

@ykadosh
Copy link
Collaborator

ykadosh commented Jun 5, 2021

I see, how about creating a small component for that? Something like:

const Details = ({children, title}) => {
    const {visible, toggle} = useVisibilityState();
    return (
        <div className='details'>
            <div className='details-header' onClick={toggle}>
                {title}
            </div>
            <Collapsible expanded={visible}>
                {children}
            </Collapsible>
        </div>
    );
};

Which you can then use like:

<Details title='click to toggle'>
    content here
</Details>

@ykadosh ykadosh added Collapsible enhancement New feature or request labels Jun 5, 2021
@yairEO
Copy link
Contributor Author

yairEO commented Jun 5, 2021

I was considering creating a Github repository only for Storybook-specific components, for common things

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Collapsible enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants