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

Support objects with <Select> #10845

Closed
1 task done
npeham opened this issue Mar 29, 2018 · 17 comments
Closed
1 task done

Support objects with <Select> #10845

npeham opened this issue Mar 29, 2018 · 17 comments
Labels
component: select This is the name of the generic UI component, not the React module!

Comments

@npeham
Copy link

npeham commented Mar 29, 2018

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

Expected Behavior

Not showing warnings when passing an object as value to the Select component.

Current Behavior

It works but it is showing these warnings:

  • Warning: Failed prop type: Invalid prop 'value' supplied to 'Select'.
  • Warning: Failed prop type: Invalid prop 'value' supplied to 'Input'.
  • Warning: Failed prop type: Invalid prop 'value' supplied to 'SelectInput'.

Steps to Reproduce (for bugs)

Thats my component in which I use the Select component. As you can see this.props.selectedWorkingStep is an object which causes those warnings.

class WorkingStepSelect extends React.Component {
    render() {
        return <FormControl error={this.props.error} className={this.props.classes.formControl}>
            <InputLabel htmlFor="workingStepSelect">{germanMessages.resources.controls.workingStepSelect.label}</InputLabel>
            <Select
                value={this.props.selectedWorkingStep}
                onChange={this.props.onChangeWorkingStep}
                input={<Input name="workingStep" id="workingStepSelect" />}
            >
                {this.props.workingSteps.map((workingStep) => {
                    return <MenuItem key={workingStep.id} value={workingStep}>{workingStep.name}</MenuItem>
                })}
            </Select>
            <FormHelperText className="error-message" error={this.props.error}>{germanMessages.resources.controls.workingStepSelect.errorMessages[this.props.error]}</FormHelperText>
        </FormControl>
    }
}

PropTypes:

WorkingStepSelect.propTypes = {
    selectedWorkingStep: PropTypes.any,
    workingSteps: PropTypes.arrayOf(PropTypes.object),
    onChangeWorkingStep: PropTypes.func,
    error: PropTypes.any,
    classes: PropTypes.object
};

CodeSandbox example

https://codesandbox.io/s/8x8r5plx32?module=%2Fsrc%2Fpages%2Findex.js

Context

Its not that critical because it is working for me(maybe there are errors which I don't encountered yet). Maybe #10834 has the same problem but I don't know it is the same reason for the warning because my warnings show up not depending on if I select an item of the Select.

Your Environment

Tech Version
Material-UI [email protected]
React [email protected]
browser Chrome Version 65
etc
@oliviertassinari oliviertassinari added component: select This is the name of the generic UI component, not the React module! waiting for user information labels Mar 29, 2018
@oliviertassinari

This comment has been minimized.

@astridcst

This comment has been minimized.

@npeham
Copy link
Author

npeham commented Mar 29, 2018

@oliviertassinari Sure: https://codesandbox.io/s/8x8r5plx32?module=%2Fsrc%2Fpages%2Findex.js

@astridcst In my codesandbox example I didn't add 'material types' and it is still showing me these warnings, so I don't think this is the problem or I just didn't get it what you mean.

@oliviertassinari oliviertassinari changed the title [SELECT] Failed prop type: Invalid prop 'value' supplied to 'Select' [Select] Failed prop type: Invalid prop 'value' supplied to 'Select' Mar 29, 2018
@oliviertassinari
Copy link
Member

oliviertassinari commented Mar 29, 2018

@npeham Thanks a lot for the reproduction example! The value proptype isn't any.
It has to be a string or a number (or an array of this). You are providing an object to the component:

  {
    id: 1,
    name: "StepOne"
  }

While it's "working", it comes with two drawbacks:

  • You can't switch on and off the native property base on your environment. For instance, native={true} works great on mobile.
  • The value can't be serialized and sent down to the server with a raw POST/GET request:
    value="[object Object]"
    capture d ecran 2018-03-29 a 22 41 40
    It's important when you are using React and Material-UI as a server-side templating engine without client-side rendering and in different other niche use cases. I also "believe" it helps with accessibility.

Here is an updated version without the warning https://codesandbox.io/s/1yvymk2r67.

Now, I think that we should something about this confusing warning message. I have seen it too personally, wtf:
Warning: Failed prop type: Invalid prop 'value' supplied to 'Select'.

@luizrrodrigues
Copy link

luizrrodrigues commented Mar 29, 2018

I have some issue, I think problem is when pass a array of object to value

This data will show warning:

[
    {id: 1, value='One'},
    {id: 2, value='Two'}
]

@oliviertassinari oliviertassinari added docs Improvements or additions to the documentation good first issue Great for first contributions. Enable to learn the contribution process. labels Mar 29, 2018
@oliviertassinari
Copy link
Member

I think that the best step going forward is to write a custom prop-type validator. The warning message isn't actionable right now:
https://github.com/mui-org/material-ui/blob/7f73f967c1f8c405eb14304de0bbd1aeae5b4d71/src/TextField/TextField.js#L261-L265

@luizrrodrigues
Copy link

luizrrodrigues commented Mar 29, 2018

@oliviertassinari About updated version, its works with single selects, but still have problem with multiple selects in renderValue*

Can't use this approach:

renderValue: selected =>
  {selected.map(value =>
    <Chip key={value.id} label={value.name} />
  )}

@oliviertassinari
Copy link
Member

oliviertassinari commented Mar 29, 2018

@luizrrodrigues

+values = {…}

renderValue: selected =>
  {selected.map(value =>
-    <Chip key={value.id} label={value.name} />
+    <Chip key={values[value].id} label={values[value].name} />
  )}

@luizrrodrigues
Copy link

luizrrodrigues commented Mar 29, 2018

@oliviertassinari

In this case can't pass .id to MenuItem, need pass index, then when pass state to POST need use map to get all IDs:

items = [
    {id: 32, name: 'John'},
    {id: 54, name: 'Mark'}
]
items.map((item, index) =>
    <MenuItem key={item.id} value={index}>
        <Checkbox checked={this.state.selectedItems.indexOf(index) > -1} />
        <ListItemText primary={item.name} />
    </MenuItem>
)

Data to POST:

itemsData: this.state.selectedItems.map(index =>
    return items[index].id
)

@oliviertassinari oliviertassinari removed the good first issue Great for first contributions. Enable to learn the contribution process. label Mar 30, 2018
@oliviertassinari oliviertassinari self-assigned this Mar 30, 2018
@oliviertassinari
Copy link
Member

Now, I think that we should something about this confusing warning message. I have seen it too personally, wtf:
Warning: Failed prop type: Invalid prop 'value' supplied to 'Select'.

After some digging, I have realized that it's a broader issue with the prop-types package. The value property isn't the only property suffering from this limitation. It's not something worse fixing here. It would be much better to fix facebook/prop-types#9.

@oliviertassinari oliviertassinari added external dependency Blocked by external dependency, we can’t do anything about it and removed docs Improvements or additions to the documentation labels Mar 30, 2018
@npeham
Copy link
Author

npeham commented Mar 30, 2018

@oliviertassinari Using the id as value for both MenuItem and Select works well. I understand now. Thank you very much!

@luizrrodrigues
Copy link

@oliviertassinari Thanks, I'll follow.

@oliviertassinari oliviertassinari changed the title [Select] Failed prop type: Invalid prop 'value' supplied to 'Select' [prop-types] Failed prop type: Invalid prop 'value' supplied to 'Select' Jun 28, 2018
@oliviertassinari oliviertassinari changed the title [prop-types] Failed prop type: Invalid prop 'value' supplied to 'Select' [prop-types] Failed prop type: Invalid prop 'value' supplied to X Jun 28, 2018
@ali-jalaal
Copy link

@oliviertassinari Sorry for commenting on closed issue, but I think 'boolean' should be defined as a valid type for 'value'. I have a Select with true / false values and I got invalid prop type warning.

@oliviertassinari
Copy link
Member

@ali-jalaal I haven't looked into the boolean. Interesting concern. As long as it's working for both the native and non-native select component, I think that we should be officiality supporting it :).

@oliviertassinari oliviertassinari removed the external dependency Blocked by external dependency, we can’t do anything about it label Oct 16, 2018
@oliviertassinari oliviertassinari changed the title [prop-types] Failed prop type: Invalid prop 'value' supplied to X Closed Support objects with <Select> Oct 16, 2018
@oliviertassinari oliviertassinari changed the title Closed Support objects with <Select> Support objects with <Select> Oct 16, 2018
@oliviertassinari oliviertassinari removed their assignment Oct 18, 2018
@don-p
Copy link

don-p commented Feb 21, 2019

@npeham Thanks a lot for the reproduction example! The value proptype isn't any.
It has to be a string or a number (or an array of this). You are providing an object to the component:

  {
    id: 1,
    name: "StepOne"
  }

While it's "working", it comes with two drawbacks:

  • You can't switch on and off the native property base on your environment. For instance, native={true} works great on mobile.
  • The value can't be serialized and sent down to the server with a raw POST/GET request:
    value="[object Object]"
    capture d ecran 2018-03-29 a 22 41 40
    It's important when you are using React and Material-UI as a server-side templating engine without client-side rendering and in different other niche use cases. I also "believe" it helps with accessibility.

Here is an updated version without the warning https://codesandbox.io/s/1yvymk2r67.

Now, I think that we should something about this confusing warning message. I have seen it too personally, wtf:
Warning: Failed prop type: Invalid prop 'value' supplied to 'Select'.

@oliviertassinari

I am seeing this issue in the latest version.

And, regarding your reply about using only string or number as value, the API for Select doesn't state this:
https://material-ui.com/api/select/


value | union: string, number, bool, object, arrayOf |   | The input value. This property is required when the nativeproperty is false (default).
-- | -- | -- | --

If the list of MenuItem elements contain an object as their value, shouldn't the Select element also accept object as value?

@joshwooding
Copy link
Member

@don-p Do you have a minimal reproducible example? I've upgraded the reproduction above to use the latest version and it works fine. https://codesandbox.io/s/kon52opp4r

@eps1lon
Copy link
Member

eps1lon commented Feb 21, 2019

And, regarding your reply about using only string or number as value, the API for Select doesn't state this:

If you care about the actual values in the input you should always manually cast them to strings. The DOM doesn't allow for non-string values on input.

The type is wider in the propTypes because most of the feedback we got didn't care about the actual input value. The value that was passed to other parts of their software seemed to be constructed manually.

This is in line with how react-dom treats the input value. Technically it should warn about non-string values but seem to recognize that most users don't rely on the value.

For typescript users the current behavior is confusing though since <input /> only accepts number | string | string[]. But even here string[] doesn't make much sense because that value is also implicitly cast to string.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: select This is the name of the generic UI component, not the React module!
Projects
None yet
Development

No branches or pull requests

8 participants