Skip to content

[Select] Add SelectDisplayProps prop#10408

Merged
oliviertassinari merged 7 commits into
mui:v1-betafrom
noah-potter:feature/add-SelectDisplayProps
Feb 23, 2018
Merged

[Select] Add SelectDisplayProps prop#10408
oliviertassinari merged 7 commits into
mui:v1-betafrom
noah-potter:feature/add-SelectDisplayProps

Conversation

@noah-potter
Copy link
Copy Markdown
Contributor

@noah-potter noah-potter commented Feb 22, 2018 •
edited
Loading

Closes #10401

@noah-potter
Copy link
Copy Markdown
Contributor Author

noah-potter commented Feb 23, 2018 •
edited
Loading

Is this error something I can resolve? It's not entirely clear what the error is.

@mbrookes
Copy link
Copy Markdown
Member

mbrookes commented Feb 23, 2018

@noah-potter run yarn docs:api or npm run docs:api

oliviertassinari
oliviertassinari previously requested changes Feb 23, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

I have one concern with exposing the SelectDisplayProps property to the top Select component. It's boilerplate the native implementation might not need. What about scoping the change to the SelectInput component waiting for the component to be split with #10335?

Comment thread src/Select/SelectInput.js Outdated
open: openProp,
readOnly,
renderValue,
SelectDisplayProps = {},
Copy link
Copy Markdown
Member

@oliviertassinari oliviertassinari Feb 23, 2018

Choose a reason for hiding this comment

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

No need for the default.

@noah-potter
Copy link
Copy Markdown
Contributor Author

noah-potter commented Feb 23, 2018

@oliviertassinari Should that scoping happen whenever SelectInput is split? I'm not sure how to make this change anticipate the two SelectInputs

@oliviertassinari oliviertassinari added type: new feature Expand the scope of the product to solve a new problem. scope: select Changes related to the select. and removed PR: needs revision labels Feb 23, 2018
@oliviertassinari
Copy link
Copy Markdown
Member

oliviertassinari commented Feb 23, 2018

I think that the simplest path to solve #10335 is to introduce a NativeSelect component, sharing some code with Select.

@oliviertassinari oliviertassinari self-assigned this Feb 23, 2018
@oliviertassinari oliviertassinari merged commit abd6cfc into mui:v1-beta Feb 23, 2018
@nikoskleidis nikoskleidis mentioned this pull request Feb 27, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@oliviertassinari oliviertassinari oliviertassinari left review comments

Assignees

@oliviertassinari oliviertassinari

Labels

scope: select Changes related to the select. type: new feature Expand the scope of the product to solve a new problem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@noah-potter @mbrookes @oliviertassinari