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

[core] Synchronise value and checked prop typing #15245

Merged
merged 22 commits into from
Apr 10, 2019

Conversation

joshwooding
Copy link
Member

@joshwooding joshwooding commented Apr 7, 2019

This allows for easier integration with redux-form without warning in the console.

const renderCheckBox = field => {
  return <Checkbox {...field.input} />;
};

instead of

const renderCheckBox = field => {
  const { input: { value, ...props} } = field;
  return <Checkbox value={String(value)} {...props} />;
};

Edit @eps1lon:
Closes #14286, #13782

@joshwooding joshwooding added the component: checkbox This is the name of the generic UI component, not the React module! label Apr 7, 2019
@mui-pr-bot
Copy link

mui-pr-bot commented Apr 7, 2019

No bundle size changes comparing 8a67ecc...96a937c

Generated by 🚫 dangerJS against 96a937c

@joshwooding joshwooding changed the title [Checkbox] Update value propTypes [core] Synchronise value prop typing Apr 9, 2019
@joshwooding joshwooding added core typescript and removed component: checkbox This is the name of the generic UI component, not the React module! labels Apr 9, 2019
@joshwooding
Copy link
Member Author

I've left the progress indicators and Slider value props as numbers

Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry that I didn't mention that at first but we should use unknown for the types. I'm taking care of this as well as looking to event.target.value for select related components

@eps1lon eps1lon self-assigned this Apr 10, 2019
@eps1lon eps1lon added on hold There is a blocker, we need to wait and removed PR: good for merge labels Apr 10, 2019
@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 10, 2019

Should we do the propTypes & TypeScript changes in two different pull requests?

@eps1lon
Copy link
Member

eps1lon commented Apr 10, 2019

They're both concerned with types. Since TypeScript is coupled with prop-types we should have them in one commit.

@oliviertassinari
Copy link
Member

I agree in general. I was wondering as each commit would create value. I'm happy to move both as a whole.

PropTypes.oneOfType([PropTypes.string, PropTypes.number, PropTypes.bool, PropTypes.object]),
),
]),
value: PropTypes.any,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At some point we should remove those. We usually don't document props from the root component. I guess keeping them here is more pragmatic. Once our docs are able to expand all props without having to keep multiple tabs open we can remove these here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, we break the rule to make the input component easier to use.

@eps1lon eps1lon closed this Apr 10, 2019
@eps1lon eps1lon reopened this Apr 10, 2019
@eps1lon
Copy link
Member

eps1lon commented Apr 10, 2019

Radio and RadioGroup are a hot mess right now since we support them without the context. Without the context passing in any value doesn't do anything but within the context non-string values won't work as expected.

If we need any value support we should implement this in another PR. Right now anything but strings is dangerous because of JS weak typing.

@eps1lon eps1lon removed the on hold There is a blocker, we need to wait label Apr 10, 2019
@@ -110,7 +110,7 @@ RadioGroup.propTypes = {
/**
* Value of the selected radio button.
*/
value: PropTypes.any,
value: PropTypes.string,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's wrong with any here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See 757384e

Copy link
Member

@oliviertassinari oliviertassinari Apr 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct. We would need to cast the values to a string in the comparison to support any:

  let checked = checkedProp;
  const onChange = createChainedFunction(onChangeProp, radioGroup && radioGroup.onChange);
  let name = nameProp;

  if (radioGroup) {
    if (typeof checked === 'undefined') {
-     checked = radioGroup.value === props.value;
+     checked = String(radioGroup.value) === String(props.value);

I believe it's the only change required.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you looked at the test?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have. My proposed change would only support string | number | boolean. But as you said, it's a topic for another pull request. Is this important? I'm not sure.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty sure boolean would break easily and number is still dangerous.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants