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

[Dropdown Menu] value propType is React.PropTypes.object #2350

Closed
tehleach opened this issue Dec 2, 2015 · 4 comments · Fixed by #2352
Closed

[Dropdown Menu] value propType is React.PropTypes.object #2350

tehleach opened this issue Dec 2, 2015 · 4 comments · Fixed by #2352
Labels
component: menu This is the name of the generic UI component, not the React module!

Comments

@tehleach
Copy link
Contributor

tehleach commented Dec 2, 2015

I think this shouldn't have a strict propType. If it must, it should be type number. In Select Fields, the example value property is an int.

In my case, I found this when I tried passing in a whole selected menuItem { payload: 1, text: "LAX" } as value to a SelectFields component. This allowed me to select a component but it wouldn't display. When I passed the selected item's payload as value, it worked properly but I received warning

Warning: Failed propType: Invalid prop `value` of type `number` supplied to `DropDownMenu`, expected `object`. Check the render method of `Filter`.

I think the fix would be to just remove the value property from Dropdown Menu's propTypes. I can make a PR if this is correct.

@tehleach tehleach changed the title [Dropdown Menu] value propType is React.PropTypes.string [Dropdown Menu] value propType is React.PropTypes.object Dec 2, 2015
@oliviertassinari
Copy link
Member

I think the fix would be to just remove the value property from Dropdown Menu's propTypes

Sorry, but I plan to move the documentation of each property from the doc to the source code.
And I also plan to enforce the eslint rule react/prop-types.

Can we instead use the oneOf option for the PropType validation?

@tehleach
Copy link
Contributor Author

tehleach commented Dec 2, 2015

Sure! Fine by me. I just wanted to be sure that I wasn't using the SelectField component improperly.

So that I can be sure, the value property should be of whatever type the valueMember property is on the menuItem, correct?

Should I go ahead with the PR to use oneOf then?

@oliviertassinari
Copy link
Member

I just wanted to be sure that I wasn't using the SelectField component improperly

It seems that you are using it as in the documentation. It should be the right way to do.

Having a look at this line https://github.com/callemall/material-ui/blob/master/src/drop-down-menu.jsx#L207.
I would say that we can set the PropType of value to any.
Could you add it to the SelectField too?

@markshust
Copy link
Contributor

edit: never mind, i had an old cached version. everything is good.

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

Successfully merging a pull request may close this issue.

4 participants