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

Expose RadioGroupContext via useRadioGroup Hook #18918

Closed
1 task done
NMinhNguyen opened this issue Dec 19, 2019 · 10 comments · Fixed by #18920 or #21910
Closed
1 task done

Expose RadioGroupContext via useRadioGroup Hook #18918

NMinhNguyen opened this issue Dec 19, 2019 · 10 comments · Fixed by #18920 or #21910
Labels
component: radio This is the name of the generic UI component, not the React module! docs Improvements or additions to the documentation ready to take Help wanted. Guidance available. There is a high chance the change will be accepted

Comments

@NMinhNguyen
Copy link
Contributor

NMinhNguyen commented Dec 19, 2019

  • I have searched the issues of this repository and believe that this is not a duplicate.

Summary 💡

Expose RadioGroupContext via useRadioGroup Hook (cf. useFormControl).

Examples 🌈

https://codesandbox.io/s/material-demo-9w4yc

Motivation 🔦

I'd like the ability to easily work out whether a wrapped Radio is checked, so I can apply custom styling to a checked label, for example (see demo above). In MUI v3, this was possible because RadioGroup would use cloneElement and pass checked prop to children. With the new context implementation (#15069), I now need to hook into the context but because it's not part of public API, I'd need to use a Babel transform to import the correct module (CJS vs ESM) in my library output:

  • @material-ui/core/RadioGroup/RadioGroupContext
  • @material-ui/core/esm/RadioGroup/RadioGroupContext

@oliviertassinari has pointed out that we're currently exposing FormControlContext via a custom useFormControl Hook, so we could probably do the same in this instance? I'm happy to raise a PR for this.

Prior art: #11865

@eps1lon eps1lon added the new feature New feature or request label Dec 19, 2019
@eps1lon
Copy link
Member

eps1lon commented Dec 19, 2019

That looks fine to me. I can't remember a situation where exposing the context as read-only has locked us into a "bad" API.

While this specific customization is doable without using RadioGroupContext I think we can allow this.

@NMinhNguyen
Copy link
Contributor Author

While this specific customization is doable without using RadioGroupContext I think we can allow this.

Oh, how would you do it instead?

@NMinhNguyen
Copy link
Contributor Author

exposing the context as read-only

Do you mean excluding values such as update functions from what's returned by useRadioGroup?

@eps1lon
Copy link
Member

eps1lon commented Dec 27, 2019

While this specific customization is doable without using RadioGroupContext I think we can allow this.

Oh, how would you do it instead?

Manually do the checked test and pass it to MyFormControlLabel: https://codesandbox.io/s/material-demo-6dbb3. It's not pretty but does the job. Likely based on personal experience whether you prefer maintaining repetition or internals of third party libraries.

@maciej-gurban
Copy link
Contributor

maciej-gurban commented Mar 31, 2020

Is there a way to hook into the RadioGroupContext or any other way to get the currently selected value from RadioGroup's value? I'm also after creating a wrapper around Radio, and not using FormControlLabel.

By importing the RadioGroupContext, and trying to use it I'm getting undefined as in the consumer. I could go with creating a custom FormControlLabel, but I don't really want nor need to create a generic one which would work for other form elements - I'm only after creating custom Radios. Or would it be better for my custom Radio to already contain FormControlLabel?

@oliviertassinari
Copy link
Member

oliviertassinari commented Mar 31, 2020

I'm reopening as I believe we miss:

  • A section in the documentation.
  • To export useRadioGroup in the barrel index (js and ts).

@oliviertassinari oliviertassinari added component: radio This is the name of the generic UI component, not the React module! ready to take Help wanted. Guidance available. There is a high chance the change will be accepted labels Mar 31, 2020
@NMinhNguyen
Copy link
Contributor Author

  • To export useRadioGroup in the barrel index (js and ts).

FYI it already is exported via https://github.com/mui-org/material-ui/blob/5aad5f2f729066534bd9d408cc8518b51a9f0f14/packages/material-ui/src/RadioGroup/index.js#L2 and https://github.com/mui-org/material-ui/blob/5aad5f2f729066534bd9d408cc8518b51a9f0f14/packages/material-ui/src/index.js#L239

so we just need a documentation change.

@oliviertassinari oliviertassinari added docs Improvements or additions to the documentation and removed new feature New feature or request labels Mar 31, 2020
@joonarahko
Copy link

joonarahko commented Apr 3, 2020

I am seeing a weird situation. When I am importing the RadioGroup and useRadioGroup like this

import RadioGroup from '@material-ui/core/RadioGroup';
import useRadioGroup from '@material-ui/core/RadioGroup/useRadioGroup';

I notice that the context is being created twice. This is because @material-ui/core/RadioGroup points to the module in the esm folder and @material-ui/core/RadioGroup/useRadioGroup points to the module in the root (cjs) target.

However, if I do

import RadioGroup, { useRadioGroup }  from '@material-ui/core/RadioGroup';

Then both modules refer to the same esm module

Basically, because they refer to modules in different places, the context created on import is also different. I cannot repro this on codesandbox, but I can repro this locally. I assume codesandbox uses UMD so this problem would not be prevalent there. Got a small hole in my knowledge as I don't know exactly why it choses to go for the cjs in the first example.

@oliviertassinari

@NMinhNguyen
Copy link
Contributor Author

NMinhNguyen commented Apr 3, 2020

I am seeing a weird situation. When I am importing the RadioGroup and useRadioGroup like this

import RadioGroup from '@material-ui/core/RadioGroup';
import useRadioGroup from '@material-ui/core/RadioGroup/useRadioGroup';

@joonarahko that works as expected, you're not supposed to import beyond the 2nd level. The second way you showed is the correct way: https://material-ui.com/guides/minimizing-bundle-size/#option-1

You may find this ESLint plugin useful (currently not published, so just paste it into your project): https://github.com/mui-org/material-ui/blob/27471b4/packages/eslint-plugin-material-ui/src/rules/restricted-path-imports.js

@joonarahko
Copy link

Ok :) Makes sense. Thanks @NMinhNguyen

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: radio This is the name of the generic UI component, not the React module! docs Improvements or additions to the documentation ready to take Help wanted. Guidance available. There is a high chance the change will be accepted
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants