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

[ToggleButton, ToggleButtonGroup] Use context to pass data between ToggleButtonGroup and ToggleButton #12961

Conversation

RobertPurcea
Copy link
Contributor

@RobertPurcea RobertPurcea commented Sep 22, 2018

One possible implementation that allows people to use ToggleButton in ToggleButtonGroup without caring about how nested ToggleButton is.

This was discussed in #12921

Let me know if this is something you might merge and I'll take care of testing as well

@eps1lon
Copy link
Member

eps1lon commented Sep 22, 2018

I'm really interested in a comparison between controller components with the cloneElement pattern and the new context api. That way we could think about rewriting every controller-like component if the context api is better suited for these types of components. If it turns out that cloneElement is fine then I would stick to that pattern for consistency sake. Didn't spend much time thinking about this so I'm interested in opinions.

@RobertPurcea Not a review but I'm glad you showed how this could look like 👍

@mbrookes
Copy link
Member

Some v0 and a few early versions of v1 components used the old context feature for this sort of thing, but they were removed prior to release of v1. @oliviertassinari might know the details.

@oliviertassinari
Copy link
Member

Answered in #12921 (comment)

@oliviertassinari
Copy link
Member

@oliviertassinari might know the details.

I don't, or I don't remember.

@eps1lon
Copy link
Member

eps1lon commented Sep 23, 2018

The old context API was documented as experimental which was probably one argument against using it.

@mbrookes
Copy link
Member

The old context API was documented as experimental which was probably one argument against using it.

No, we use it for other things, even still (e.g the theme), so it wasn't that.

@oliviertassinari oliviertassinari added the component: toggle button This is the name of the generic UI component, not the React module! label Sep 24, 2018
@mbrookes
Copy link
Member

mbrookes commented Oct 9, 2018

What was the decision in the end?

@brennancheung
Copy link

What's the status of this? I see the PR has a solution but looks like the tests weren't updated.

Has there been a final verdict on whether using the Context API is an acceptable solution? Seems like it's more flexible and it has my vote.

I'm seeing the Tooltip problems and will probably create a wrapping component to push the props through in the meanwhile but would rather have it work out of the box. Context API really seems like the way to go.

@eps1lon
Copy link
Member

eps1lon commented Dec 7, 2018

This is in my mind the accepted solution but if this gets pushed forward it should include a more generic component that can also be used in RadioGroup and possibly Select.

@oliviertassinari
Copy link
Member

oliviertassinari commented Dec 10, 2018

The new hook API reduces the overhead from using the context, at least from a boilerplate point of view. I don't know about the runtime implications. I see pros and cons going both directions. I won't take a side here, if not the side that requires doing less.

@eps1lon You have mentioned the Select, as far as I know, the issue is at a different level. We need to rewrite the focus handling logic: #13708.

@eps1lon
Copy link
Member

eps1lon commented Dec 11, 2018

@oliviertassinari The component I'm talking would only take care about handling what items are selected in a collection. Focus is a different concern and should therefore be handled in a different component.

@joshwooding
Copy link
Member

I would vote for using context, it's already powerful and hooks make using it even better 🥇

@eps1lon
Copy link
Member

eps1lon commented Dec 19, 2018

I made a basic implementation a few months ago which used render props but never finished it: eps1lon@7df852f

Someone can pick this up and port this to react context instead of render props API. It's a pretty common concern so it would be nice if we could use it in ToggleButtonGroup´, RadioGroup, Menu` etc. Needs further investigation though.

@joshwooding joshwooding self-assigned this Feb 10, 2019
@joshwooding
Copy link
Member

I've been playing with this https://github.com/joshwooding/material-ui/tree/selectable-group
https://joshuawooding-mui.netlify.com/lab/selectable-group/

I still need to investigate integrating it with ToggleButtonGroup and other components.

@joshwooding
Copy link
Member

joshwooding commented Feb 19, 2019

Closing in favour of #14585

Thanks for the inspiration @RobertPurcea
Thanks for the starting point @eps1lon

@joshwooding joshwooding removed their assignment Feb 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: toggle button This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants