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

[IconMenu] Add open, onRequestOpen, onRequestClose properties, fix #2341 #2343

Closed
wants to merge 2 commits into from

Conversation

quangbuule
Copy link
Contributor

No description provided.

@quangbuule quangbuule changed the title [IconMenu] Add open, onRequestOpen, onRequestClose properties #2341 [IconMenu] Add open, onRequestOpen, onRequestClose properties, fix #2341 Dec 2, 2015
@alitaheri
Copy link
Member

Nice work 👍
However, the convention around this library for declarative api is to use onRequestChange with requested open state change as argument. Also, since there can be more reasons why open=false is requested the better api would be: onRequestChange(open, reason). where reason can be: 'itemClick', 'clickaway' or else. This makes the usage a lot easier as people can just do:

onRequestChange={(open, reason) => { if (reason === 'clickaway') this.setState({open}); }}

@oliviertassinari oliviertassinari modified the milestones: Composable Components, Declarative API Dec 2, 2015
if (process.env.NODE_ENV !== 'production') {
this._warningIfNeeded();
}

Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this update the open state depending the prop.open in the getInitialState method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I forgot about this.

@oliviertassinari
Copy link
Member

Yeah, nice work. Could you take into account @subjectix feedback?

@oliviertassinari oliviertassinari added 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 Dec 2, 2015
@quangbuule
Copy link
Contributor Author

Btw, onRequestClose and onRequestOpen followed Dialog convention. Should we do onRequestChange on this PR?

@oliviertassinari
Copy link
Member

Well, that's a good question!
To sum up we have:

  • for the Dialog: onRequestClose(buttonClicked)
  • for the Popover: onRequestClose(reason)
  • for the LeftNav: onRequestChange(open, reason)
  • for this component I would say onRequestChange(open, reason) because we can either resquest a close or an open event.

Sure, we could improve the consitency. Though? @shaurya947 @subjectix @chrismcv

@quangbuule
Copy link
Contributor Author

It's okie with onRequestChange. But there is something I concern about naming.

  1. If onRequestChange means when "request" change, So what is request, and it seems we are having current request value, and it's changing, in my head, request in this case is an event, not a value. So the onRequestChange make me confused a little bit.
  2. If onRequestChange is just simple the Component is willing to change something, then it should wrap onChange, it's confusing anyway.

That's my concern, but I am okie with our convention :).

@oliviertassinari
Copy link
Member

I would say that it's better to use onRequestChange(open, reason) here.

@quangbuule
Copy link
Contributor Author

Okie. Another question: what is the reason when user select menuItem by Enter key? menuItem or keyboard?

@alitaheri
Copy link
Member

onChange somehow means: This component was change, you may do something about it if you wish not the factual This component was demanded to change it's state. If you wish, you can change the prop value you provided to react to the change. However, I can't argue that onRequestChange is indeed confusing by itself. @quangbuule you sure challenge the conventions good :D :D
For this PR keep all the conventions intact. we have to make some changes all over the place to address this.

@quangbuule Do the honor and open 2 issues regarding both onRequestChange and default: undefined. Your arguments are valid, but we need the community's feedback on this.

@quangbuule
Copy link
Contributor Author

@subjectix So what do you think about this:

<input value={this.state.text} onChange={{} => {/* I accepted the factual by doing nothing here */} />

Will the input value be changed after onChange?
Sorry about arguing too much on this :P

@oliviertassinari
Copy link
Member

Well, it depend on how you intreprete the different properties of https://facebook.github.io/react/docs/two-way-binding-helpers.html

@quangbuule
Copy link
Contributor Author

It's okie for me with onRequestChange and the null open in this PR. We will have issues on naming later. @subjectix @oliviertassinari, can you take a look at my above question?
#2343 (comment)

@alitaheri
Copy link
Member

@quangbuule Well, I myself like onChange a lot more, it's a lot easier for me to write. But the community must decide. Open issues and lets discuss them with everyone. You have some great ideas 👍 👍
About your question. I think 'enter' would do. Thoughts @oliviertassinari ?

@oliviertassinari
Copy link
Member

what is the reason when user select menuItem by Enter key? menuItem or keyboard?

Why not just calling it enter?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants