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

breaking(Dropdown): add options shorthand #1038

Closed
wants to merge 9 commits into from

Conversation

levithomason
Copy link
Member

@levithomason levithomason commented Dec 16, 2016

TODO

  • Fix bug with allow additions examples (key warning, no addition added)
  • Change SSOT for getting items to return props
  • fix bug with duplicate keys (i.e. fix tests)

Breaking Changes!

This PR updates the Dropdown to allowing defining options with any shorthand value: number, string, props object, or another element.

options prop renamed to items

As a design decision, shorthand props are provided for each subcomponent. Example, a Modal with <Modal.Header>Foo</Modal.Header> can be configured as <Modal header='Foo' />. The Dropdown optionsprop is used to create<Dropdown.Item />s, hence, the correct shorthand prop name is items`.

Before

const options = [{ key: 1, text: 'One', value: 1 }, ...]
<Dropdown options={options} />

After

const items = [{ key: 1, text: 'One', value: 1 }, ...]
<Dropdown items={items} />

New search callback signature

The items are used to create <DropdownItem />s internally. Custom search functions, which used to receive the original items objects, now receive <DropdownItem /> instances. Use the instance's props to access the text, value, and other props as necessary.

Before

const options = [{ key: 1, text: 'One', value: 1 }, ...]

const search = (options, query) => {
  // options = [{ key: 1, text: 'One', value: 1 }, ...]
  return options.filter(opt => opt.text === query)
}

<Dropdown search={search} options={options} />

After

const items = [{ key: 1, text: 'One', value: 1 }, ...{}]

const search = (options, query) => {
  // items = [<Dropdown.Item key={1} text='One' value=1 />, ...]
  return items.filter(item => item.props.text === query)
}

<Dropdown search={search} items={items} />

New renderLabel callback signature

Multiple select Dropdowns can render a custom Label for each active DropdownItem. This callback, which used to receive a single option object, now receives a <DropdownItem /> instance. Use the instance's props to access the text, value, and other props as necessary.

Before

const options = [{ key: 1, text: 'One', value: 1 }, ...]

const renderLabel = (item, index, defaultLabelProps) => {
  // item = { key: 1, text: 'One', value: 1 }
  return defaultLabelProps
}

<Dropdown renderLabel={renderLabel} options={options} />

After

const items = [{ key: 1, text: 'One', value: 1 }, ...]

const renderLabel = (item, index, defaultLabelProps) => {
  // item = <Dropdown.Item key={1} text='One' value={1} />
  return defaultLabelProps
}

<Dropdown renderLabel={renderLabel} items={items} />

Fixes #470

This adds shorthand factory support to the Dropdown options.

@levithomason levithomason changed the title feat(Dropdown): feat(Dropdown): add options shorthand Dec 16, 2016
PropTypes.arrayOf(PropTypes.shape(DropdownItem.propTypes)),
]),
/** Array of Dropdown.Item shorthand */
options: customPropTypes.itemShorthand,
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be collectionShorthand

@jeffcarbs
Copy link
Member

jeffcarbs commented Dec 16, 2016

This is going to affect methods like:

getItemByValue = (value) => {
  const { options } = this.props

  return _.find(options, { value })
}

I kinda think we're gonna need to:

  • update anywhere that's using this.props.options to use this.getMenuOptions()
  • update getMenuOptions to return an array of DropdownItem
  • update anything using getMenuOptions to use option.props instead of just option

Passing it through the factory first would allow us to predictably access a prop (text, value, whatever) of an item.

Also if we do it this way, maybe update getMenuOptions to getDropdownItems to be more clear as to what it's doing.

@levithomason
Copy link
Member Author

Thanks for the extra eyes. Indeed, I'll be sure to make these changes prior to merging. This PR is half baked atm.

@UnbrandedTech
Copy link
Contributor

I'm not sure this is the place, but react-widgets has a nice ish API allowing options to be an array of objects and two optional props. One to specify the display value and another for the value to be selected.

Const people = [{id:1, name:"James"},{id:2, name:"Bob"}]
<Dropdown valueField="id" textField="name" data={people} \>

@levithomason
Copy link
Member Author

@UnbrandedTech use content for the display value in the menu and text for the active value.

http://react.semantic-ui.com/modules/dropdown#item-content

@codecov-io
Copy link

codecov-io commented Jan 13, 2017

Codecov Report

Merging #1038 into master will decrease coverage by -0.08%.
The diff coverage is 98.41%.

@@            Coverage Diff             @@
##           master    #1038      +/-   ##
==========================================
- Coverage   99.74%   99.66%   -0.08%     
==========================================
  Files         140      140              
  Lines        2345     2396      +51     
==========================================
+ Hits         2339     2388      +49     
- Misses          6        8       +2
Impacted Files Coverage Δ
src/modules/Dropdown/DropdownHeader.js 100% <100%> (ø)
src/modules/Dropdown/DropdownItem.js 100% <100%> (ø)
src/modules/Dropdown/Dropdown.js 99.48% <98.36%> (-0.52%)
src/collections/Form/FormSelect.js 100% <ø> (ø)
src/collections/Form/FormGroup.js 100% <ø> (ø)
src/modules/Accordion/Accordion.js 100% <ø> (ø)
src/modules/Accordion/AccordionContent.js 100% <ø> (ø)
src/collections/Menu/MenuItem.js 100% <ø> (ø)
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4dbaaf0...b4040bc. Read the comment docs.

@levithomason levithomason force-pushed the feat/dropdown-item-shorthand branch 3 times, most recently from 0fb5701 to 3c6a438 Compare January 25, 2017 07:24
@levithomason levithomason changed the title feat(Dropdown): add options shorthand breaking(Dropdown): add options shorthand Jan 25, 2017
@levithomason
Copy link
Member Author

Closing this PR as it has gotten too stale. Also, I'm not sure this work is worth it. The gains are minor and the repercussions are major.

@levithomason levithomason deleted the feat/dropdown-item-shorthand branch July 18, 2017 02:00
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.

Dropdown: support an array of string|element options
4 participants