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.

Sign up for GitHub

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

Jump to bottom

[Select] Add open, onClose and onOpen properties #9844

Merged
merged 2 commits into from Jan 14, 2018
Merged

[Select] Add open, onClose and onOpen properties #9844

merged 2 commits into from Jan 14, 2018

Conversation

caub
Copy link
Contributor

@caub caub commented Jan 12, 2018 •
edited

alternative for #9839

todo tests

@caub caub force-pushed the select-open branch 2 times, most recently from 5b74334 to 984c496 Compare January 12, 2018 01:55
@oliviertassinari oliviertassinari mentioned this pull request Jan 12, 2018
Closed
oliviertassinari
oliviertassinari reviewed Jan 12, 2018
View reviewed changes
Copy link
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.

It's a good start :)

src/Select/SelectInput.js Outdated
@@ -27,10 +27,13 @@ class SelectInput extends React.Component {
});
};

handleClose = () => {
handleClose = e => {
Copy link
Member

Choose a reason for hiding this comment

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

event

src/Select/SelectInput.js Outdated
@@ -269,7 +273,7 @@ class SelectInput extends React.Component {
<Menu
id={`menu-${name || ''}`}
anchorEl={this.state.anchorEl}
open={this.state.open}
open={open !== undefined ? open : this.state.open}
Copy link
Member

Choose a reason for hiding this comment

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

It would be great to use the same logic as with the other components and with how React is handling the topic internally:
https://github.com/mui-org/material-ui/blob/33d0954b8626528fda6eeaa8cc39216ccc3117dd/src/internal/SwitchBase.js#L41

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, ok, working on it, adding tests

isControlled = this.props.open !== undefined;

@oliviertassinari oliviertassinari mentioned this pull request Jan 12, 2018
Closed
@oliviertassinari oliviertassinari added the component: select This is the name of the generic UI component, not the React module! label Jan 12, 2018
@caub caub force-pushed the select-open branch 2 times, most recently from a72d373 to b7736ec Compare January 12, 2018 17:01
@caub
Copy link
Contributor Author

caub commented Jan 12, 2018 •
edited by zannager

I don't know if you'll like it, because I've not seen it used elsewhere, but I'd like to propose onToggle for callbacks, it's like onClose + onOpen + possibility to toggle (like classList.toggle), so 2 and a half features in one

also why isn't onChange listed there https://material-ui.com/api/select/ ?

@caub caub force-pushed the select-open branch 5 times, most recently from 319c1c9 to 45c3924 Compare January 12, 2018 17:49
@caub caub changed the title [Select] props: open, onOpen, onClose [Select] props: open, onChange Jan 12, 2018
@caub caub changed the title [Select] props: open, onChange [Select] props: open, onToggle Jan 12, 2018
@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 13, 2018 •
edited

I'd like to propose onToggle for callbacks, it's like onClose + onOpen + possibility to toggle (like classList.toggle)

@caub If we go down this path, we gonna need to reflect it on all our components. We need to keep the API predictable. Some component only needs the onClose callback. Personally, I prefer the verbosity and the extra control two different callbacks offer.

also why isn't onChange listed there https://material-ui.com/api/select/ ?

I have no clue 😮 . You raise a good issue!

@oliviertassinari oliviertassinari added new feature New feature or request PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI labels Jan 13, 2018
oliviertassinari
oliviertassinari reviewed Jan 13, 2018
View reviewed changes
docs/src/pages/demos/selects/MultipleSelect.js Outdated
@@ -98,6 +102,7 @@ class MultipleSelect extends React.Component {
))}
</Select>
</FormControl>

Copy link
Member

Choose a reason for hiding this comment

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

We never use a blank line in our JSX. Let's reduce entropy 👼 .

docs/src/pages/demos/selects/MultipleSelect.js Outdated
multiple
open={this.state.open}
value={this.state.nameWithClose}
onChange={e => this.setState({ nameWithClose: e.target.value })}
Copy link
Member

Choose a reason for hiding this comment

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

We encourage people to be more explicit: e -> event.

src/Select/SelectInput.js
@@ -18,25 +18,37 @@ class SelectInput extends React.Component {

ignoreNextBlur = false;

isControlled = this.props.open !== undefined;
Copy link
Member

Choose a reason for hiding this comment

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

Wow, I thought we needed to wait for the constructor call. Maybe not after all!

src/Select/SelectInput.js Outdated
@@ -351,6 +366,23 @@ SelectInput.propTypes = {
* @ignore
*/
onFocus: PropTypes.func,
/**
* Callback fired when the component requests to be closed.
Copy link
Member

Choose a reason for hiding this comment

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

The description is wrong.

@caub caub changed the title [Select] props: open, onToggle [Select] props: open, onClose, onOpen Jan 14, 2018
@caub
Copy link
Contributor Author

caub commented Jan 14, 2018 •
edited

Thanks for the notes @oliviertassinari

Which are the other Components to handle? I've checked Snackbar, Modal, Dialog, .. they are manageable already

also isn't it repetitive to have both TypeScript defs and PropTypes? maybe it's under change

one non-important thing I noticed, Select use Backdrops, it's nice, but if you have 2 selects next to each other, you'd need 2 click to switch from one to the other (instead of 1 with native selects), maybe it could be configurable (with Backdrop or not), but not important again.

@oliviertassinari oliviertassinari changed the title [Select] props: open, onClose, onOpen [Select] Add open, onClose and onOpen properties Jan 14, 2018
@oliviertassinari oliviertassinari removed the PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI label Jan 14, 2018
@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 14, 2018 •
edited

Which are the other Components to handle? I've checked Snackbar, Modal, Dialog, .. they are manageable already

@caub We are good.

also isn't it repetitive to have both TypeScript defs and PropTypes? maybe it's under change

We haven't found a better tradeoff yet.

instead of 1 with native selects

How do you reproduce this statement? I can't.

@oliviertassinari oliviertassinari merged commit 6642839 into mui:v1-beta Jan 14, 2018
@oliviertassinari
Copy link
Member

@caub Thanks

@caub
Copy link
Contributor Author

caub commented Jan 14, 2018

How do you reproduce this statement? I can't.

Oh.. You are right, ignore it :))

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
No one assigned
Labels
component: select This is the name of the generic UI component, not the React module! new feature New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@caub @oliviertassinari