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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Radio] Add size="small" support #18479

Closed
2 tasks done
lp247 opened this issue Nov 21, 2019 · 12 comments 路 Fixed by #18688
Closed
2 tasks done

[Radio] Add size="small" support #18479

lp247 opened this issue Nov 21, 2019 · 12 comments 路 Fixed by #18688
Assignees
Labels
component: checkbox This is the name of the generic UI component, not the React module! component: radio This is the name of the generic UI component, not the React module! good first issue Great for first contributions. Enable to learn the contribution process. new feature New feature or request

Comments

@lp247
Copy link

lp247 commented Nov 21, 2019

  • The issue is present in the latest release.
  • I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 馃槸

Property size is not working on component <Radio />.

Expected Behavior 馃

The docs for the Radio component state that

Any other props supplied will be provided to the root element (IconButton)

The IconButton component has a property size, so setting size on Radio should work as well. The typescript typings seem to be correct as size is recognized as valid property of Radio.

Steps to Reproduce 馃暪

This sandbox demonstrates the issue. You can set size="small" on an IconButton, but has no effect on Radio.

Tech Version
Material-UI 4.6.1
React 16.12.0
Browser Firefox v70.0.1
TypeScript 3.8.0-dev.20191121
@lp247 lp247 changed the title Property size not working on Radio component Property "size" not working on Radio component Nov 21, 2019
@oliviertassinari oliviertassinari added component: checkbox This is the name of the generic UI component, not the React module! component: radio This is the name of the generic UI component, not the React module! new feature New feature or request good first issue Great for first contributions. Enable to learn the contribution process. labels Nov 21, 2019
@oliviertassinari
Copy link
Member

oliviertassinari commented Nov 21, 2019

@pete-lennart You can find two demos for the size small variant:

However, there is no out-of-the-box prop support. I think that we could add it. What do you think of such a diff? It could be the beginning of a pull request is you are interested :).

diff --git a/docs/src/pages/components/radio-buttons/RadioButtons.tsx b/docs/src/pages/components/radio-buttons/RadioButtons.tsx
index ac30f0467..fd7ff9d72 100644
--- a/docs/src/pages/components/radio-buttons/RadioButtons.tsx
+++ b/docs/src/pages/components/radio-buttons/RadioButtons.tsx
@@ -60,8 +60,7 @@ export default function RadioButtons() {
         color="default"
         name="radio-button-demo"
         inputProps={{ 'aria-label': 'E' }}
-        icon={<RadioButtonUncheckedIcon fontSize="small" />}
-        checkedIcon={<RadioButtonCheckedIcon fontSize="small" />}
+        size="small"
       />
     </div>
   );
diff --git a/packages/material-ui/src/Radio/Radio.d.ts b/packages/material-ui/src/Radio/Radio.d.ts
index be2d98a50..5cfeb2fde 100644
--- a/packages/material-ui/src/Radio/Radio.d.ts
+++ b/packages/material-ui/src/Radio/Radio.d.ts
@@ -7,6 +7,7 @@ export interface RadioProps
   checkedIcon?: React.ReactNode;
   color?: 'primary' | 'secondary' | 'default';
   icon?: React.ReactNode;
+  size?: 'small' | 'medium';
 }

 export type RadioClassKey = SwitchBaseClassKey | 'colorPrimary' | 'colorSecondary';
diff --git a/packages/material-ui/src/Radio/Radio.js b/packages/material-ui/src/Radio/Radio.js
index 5fc1a3eb9..800d33dbd 100644
--- a/packages/material-ui/src/Radio/Radio.js
+++ b/packages/material-ui/src/Radio/Radio.js
@@ -64,6 +64,7 @@ const Radio = React.forwardRef(function Radio(props, ref) {
     disabled = false,
     name: nameProp,
     onChange: onChangeProp,
+    size = 'medium',
     ...other
   } = props;
   const radioGroup = React.useContext(RadioGroupContext);
@@ -85,8 +86,10 @@ const Radio = React.forwardRef(function Radio(props, ref) {
     <SwitchBase
       color={color}
       type="radio"
-      icon={defaultIcon}
-      checkedIcon={defaultCheckedIcon}
+      icon={React.cloneElement(defaultIcon, { fontSize: size === 'small' ? 'small' : 'default' })}
+      checkedIcon={React.cloneElement(defaultCheckedIcon, {
+        fontSize: size === 'small' ? 'small' : 'default',
+      })}
       classes={{
         root: clsx(classes.root, classes[`color${capitalize(color)}`]),
         checked: classes.checked,
@@ -160,6 +163,11 @@ Radio.propTypes = {
    * If `true`, the `input` element will be required.
    */
   required: PropTypes.bool,
+  /**
+   * The size of the radio.
+   * `small` is equivalent to the dense radio styling.
+   */
+  size: PropTypes.oneOf(['small', 'medium']),
   /**
    * The input component prop `type`.
    */
diff --git a/packages/material-ui/src/Radio/RadioButtonIcon.js b/packages/material-ui/src/Radio/RadioButtonIcon.js
index 9cb07f681..d7e8dc464 100644
--- a/packages/material-ui/src/Radio/RadioButtonIcon.js
+++ b/packages/material-ui/src/Radio/RadioButtonIcon.js
@@ -33,26 +33,20 @@ export const styles = theme => ({
  * @ignore - internal component.
  */
 function RadioButtonIcon(props) {
-  const { checked, classes } = props;
+  const { checked, classes, fontSize } = props;

   return (
     <div className={clsx(classes.root, { [classes.checked]: checked })}>
-      <RadioButtonUncheckedIcon />
-      <RadioButtonCheckedIcon className={classes.layer} />
+      <RadioButtonUncheckedIcon fontSize={fontSize} />
+      <RadioButtonCheckedIcon fontSize={fontSize} className={classes.layer} />
     </div>
   );
 }

 RadioButtonIcon.propTypes = {

It would make it consistent with the switch size support: https://material-ui.com/components/switches/#sizes.

@lp247
Copy link
Author

lp247 commented Nov 22, 2019

Yes, that looks good!

@oliviertassinari
Copy link
Member

@pete-lennart Ok great. If you want to work on a pull request, you are free to go.

@SandraMarcelaHerreraArriaga
Copy link
Contributor

Somebody has taken this issue?

@mbrookes
Copy link
Member

@SandraMarcelaHerreraArriaga Not so far. Would you like to work on it?

@SandraMarcelaHerreraArriaga
Copy link
Contributor

@SandraMarcelaHerreraArriaga Not so far. Would you like to work on it?

Sure! Thanks :) I will start working on it

@oliviertassinari oliviertassinari changed the title Property "size" not working on Radio component [Radio] Add size="small" support Nov 30, 2019
@SomervilleTom
Copy link

SomervilleTom commented Mar 31, 2021

The documentation strongly implies that ButtonGroup accepts "small" as a value of its size property. The same is true for the Radio element. I've provided that value in both places and it has no visible effect whatsoever.

Here is the code I'm using (copied from the example in the docs):

<FormControl component="fieldset" >
  <FormLabel component="legend">Data to display</FormLabel>
  <RadioGroup
    size="small" 
    name="dataSource"
    value={dataSource}
    onChange={handleDataSourceChange}
  >
    <FormControlLabel value="CaseCount" control={<Radio />} label="CaseCount" />
    <FormControlLabel value="DeathCount" control={<Radio />} label="DeathCount" />
    <FormControlLabel value="HotSpots" control={<Radio />} label="HotSpots" />
  </RadioGroup>
</FormControl>

Does anybody actually check to see that an issue like this actually works before closing it? I'm not trying to bust anybody's chops, I'm just trying to learn how to get my material-ui app working without spending days reverse engineering every last control of the UIX. One of my biggest challenges is discovering what parts of material-ui actually do and do not work as described in the documentation.

Attached is a screenshot of the result.

broken-radio-button-group

@oliviertassinari
Copy link
Member

oliviertassinari commented Mar 31, 2021

@SomervilleTom Interesting, this issue was about <Radio size="small">. Do you want to create a new issue for RadioGroup ? I think that it indeed makes a lot of sense :)

@SomervilleTom
Copy link

SomervilleTom commented Mar 31, 2021

Heh. Silly me, I thought (hoped?) they were synonymous. FWIW, I did try passing small in each of the Radio children of the RadioGroup, with no visible effect. I've just discovered the "Density" section of the docs, so I I'll try fiddling with the code some more.

@SomervilleTom
Copy link

I seem to be back at this issue. I really do want a smaller Radio component. I have provided size="small" to the Radio component (I have three of them). This results in a 24 pixel height for the resulting element, even with margin and padding of 0.

I want this design element to have a 19 pixel height (to match the standard html input and label it replaces). The input element it replaces is 13x13 (with mt=3, mr=3, and ml=5). The label it replaces has a height of 15 pixels.

The devtool inspector tells me that the svg element is 24x24. Is there a way to adjust the Radio element so that it has a 13x13 icon and a 19px height?

@oliviertassinari
Copy link
Member

@SomervilleTom
Copy link

@oliviertassinari: Way cool, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: checkbox This is the name of the generic UI component, not the React module! component: radio This is the name of the generic UI component, not the React module! good first issue Great for first contributions. Enable to learn the contribution process. new feature New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants