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

Simplified getMenuItem so that arrays of children can be passed in. #22

Merged
merged 1 commit into from
Jun 7, 2017
Merged

Simplified getMenuItem so that arrays of children can be passed in. #22

merged 1 commit into from
Jun 7, 2017

Conversation

AlastairTaft
Copy link
Contributor

This change allows you to do the following

<ContextMenu>
  {['Menu 1', 'Menu 2'].map(item => <Item>{item}</Item>)}
</ContextMenu>

@fkhadra
Copy link
Owner

fkhadra commented Jun 6, 2017

Hello @AlastairTaft,

Appreciate your contribution.

I'm wondering if there is any benefit using your method. If I'm not mistaken we can't set the onClick props to the Item component in that case. Correct me if I'm wrong.

<ContextMenu>
  {['Menu 1', 'Menu 2'].map(item => <Item>{item}</Item>)}
</ContextMenu>

@AlastairTaft
Copy link
Contributor Author

If I understand correctly you could still do something like this if you want to pass in an onClick method for each item

<ContextMenu>
  {['Menu 1', 'Menu 2'].map(item => <Item 
    onClick={() => this.handleOnClick(item)}
  >
    {item}
  </Item>)}
</ContextMenu>

As far as the change goes I don't think it is altering any existing behaviour. I don't think you need to do an array check because the React.Children methods figure out if it's a single element or collection, therefor as long as you stick to the React methods and don't use native array methods you don't need to check for the difference.

@fkhadra
Copy link
Owner

fkhadra commented Jun 7, 2017

@AlastairTaft I was confused by the snippet but after reading your code more carefully I totally agree with you 😁

@fkhadra fkhadra merged commit 78137d7 into fkhadra:master Jun 7, 2017
@AlastairTaft
Copy link
Contributor Author

Thanks for merging :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants