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

Support multiple dropdown type #254

Merged
merged 21 commits into from
Jun 10, 2016
Merged

Conversation

levithomason
Copy link
Member

@levithomason levithomason commented Jun 3, 2016

This PR adds support for the multiple dropdown type. A multiple dropdown:

  • accepts an array for the value (opposed to string/number)
  • logs errors when !production if value is wrong type for the given dropdown type
  • does not close when selecting a value
  • allows removing values by clicking the × icon on the label for a value

Other updates and fixes:

  • calls onChange with (e, value)
  • only show "no results" message if search

@codecov-io
Copy link

codecov-io commented Jun 3, 2016

Current coverage is 85.23%

Merging #254 into master will increase coverage by 1.06%

@@             master       #254   diff @@
==========================================
  Files            62         62          
  Lines           752        806    +54   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits            633        687    +54   
  Misses          119        119          
  Partials          0          0          

Powered by Codecov. Last updated by 3c58e1b...e5af50b

// convert the value to/from an array
const newValue = e.target.checked ? _.compact([value]) : value[0]
this.setState({ multiple: e.target.checked, value: newValue })
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Since this example allows toggling the multiple prop, we need to convert the value to and from an array corresponding to the dropdown type.

@eanplatter
Copy link
Contributor

🍂 sqursh enrd rbs tur maestr


const _detail = detail || detailLink
const detailComponent = (detailLink || onClickDetail) && 'a' || detail && 'div'
const DetailComponent = (detailLink || onDetailClick) && 'a' || 'div'
Copy link
Member

Choose a reason for hiding this comment

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

Would be nice to wrap the latter check in () for similar clarity to the initial expression.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not seeing where the extra parens can be added here. Here is another way of writing this:

let DetailComponent = 'div'
if (detailLink || onDetailClick) DetailComponent = 'a'

@dvdzkwsk
Copy link
Member

dvdzkwsk commented Jun 8, 2016

👻 LGTM, will pull and test locally if you want before merging.

@levithomason levithomason merged commit 0849112 into master Jun 10, 2016
@levithomason levithomason deleted the feature/multiple-dropdown branch June 10, 2016 15:28
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.

4 participants