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

feat(Dropdown): support disabled items #482

Merged
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions docs/app/Examples/modules/Dropdown/States/DisabledItem.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
import _ from 'lodash'
import faker from 'faker'
import React from 'react'
import { Dropdown } from 'stardust'

const options = _.times(10, (i) => {
const name = faker.name.findName()
return { text: name, value: _.snakeCase(name), disabled: i % 3 === 0 }
})

const DropdownItemDisabledExample = () => (
<Dropdown text='Dropdown' options={options} />
)

export default DropdownItemDisabledExample
7 changes: 6 additions & 1 deletion docs/app/Examples/modules/Dropdown/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -43,9 +43,14 @@ const DropdownExamples = () => (
<ExampleSection title='States'>
<ComponentExample
title='Disabled'
description='A disabled dropdown menu or item does not allow user interaction'
description='A disabled dropdown menu does not allow user interaction'
examplePath='modules/Dropdown/States/Disabled'
/>
<ComponentExample
title='Disabled Item'
description='A disabled dropdown item does not allow user interaction'
examplePath='modules/Dropdown/States/DisabledItem'
/>
Copy link
Member

Choose a reason for hiding this comment

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

Let's update the Disabled description to include "menu or item" and drop the title/description on the disabled item example. This way they show as two examples under one heading:

image

</ExampleSection>
</div>
)
Expand Down
20 changes: 15 additions & 5 deletions src/modules/Dropdown/Dropdown.js
Original file line number Diff line number Diff line change
Expand Up @@ -385,10 +385,10 @@ export default class Dropdown extends Component {

selectHighlightedItem = (e) => {
const { multiple, onAddItem, options } = this.props
const value = _.get(this.getSelectedItem(), 'value')
const { value, disabled } = this.getSelectedItem() || {}
Copy link
Member

@levithomason levithomason Sep 9, 2016

Choose a reason for hiding this comment

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

Am I correct in thinking you cannot actually highlight (select) a disabled item due to the logic added to the moveSelectionBy? If so, this method should never be called with a disabled item as it is only called on enter and blur and requires first using keyboard nav to highlight the item.


It's beyond the scope of this PR, but It could be this method name needs updated. selected means highlighted, active means it is part of the value. selectHighlightedItem is a little redundant as the highlighted item is already selected. It might be more accurately named makeSelectedItemActive or similar.

Copy link
Member Author

Choose a reason for hiding this comment

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

If the first item is disabled, it will be "selected" by default since the selectedIndex is set to 0.

One option to fix this would be to have a getFirstValidItem function or something (open to a better name) that would give the index of the first non-disabled item. If none (e.g. all options disabled) the selected index could be -1.


// prevent selecting null if there was no selected item value
if (!value) return
// prevent selecting null (if there was no selected item value) or disabled options
if (!value || disabled) return

// notify the onAddItem prop if this is a new value
if (onAddItem && !_.some(options, { text: value })) onAddItem(value)
Expand Down Expand Up @@ -458,14 +458,17 @@ export default class Dropdown extends Component {
debug('handleItemClick()')
debug(value)
const { multiple, onAddItem, options } = this.props
const item = this.getItemByValue(value) || {}

// prevent toggle() in handleClick()
e.stopPropagation()
// prevent closeOnDocumentClick() if multiple
if (multiple) {
// prevent closeOnDocumentClick() if multiple or item is disabled
if (multiple || item.disabled) {
e.nativeEvent.stopImmediatePropagation()
}

if (item.disabled) return

// notify the onAddItem prop if this is a new value
if (onAddItem && !_.some(options, { value })) onAddItem(value)

Expand Down Expand Up @@ -631,13 +634,19 @@ export default class Dropdown extends Component {
const options = this.getMenuOptions()
const lastIndex = options.length - 1

// Prevent infinite loop
if (_.every(options, 'disabled')) return

// next is after last, wrap to beginning
// next is before first, wrap to end
let nextIndex = selectedIndex + offset
if (nextIndex > lastIndex) nextIndex = 0
else if (nextIndex < 0) nextIndex = lastIndex

this.setState({ selectedIndex: nextIndex })

if (options[nextIndex].disabled) return this.moveSelectionBy(offset)
Copy link
Member

Choose a reason for hiding this comment

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

This will set state every time it skips a disabled item. It would be ideal to only set state once a valid next index was found. What if we also allow passing a startIndex?

moveSelectionBy = (offset, startIndex = this.state.selectedIndex) => {
  // update selectedIndex usage
}

Then we can recurse before setting state:

if (options[nextIndex].disabled) return this.moveSelectionBy(offset, nextIndex)

this.setState({ selectedIndex: nextIndex })


this.scrollSelectedItemIntoView()
}

Expand Down Expand Up @@ -819,6 +828,7 @@ export default class Dropdown extends Component {
selected={selectedIndex === i}
onMouseDown={e => e.preventDefault()} // prevent default to allow item select without closing on blur
{...opt}
style={{ ...opt.style, pointerEvents: 'all' }} // Needed for handling click events on disabled items
/>
))
}
Expand Down
5 changes: 5 additions & 0 deletions src/modules/Dropdown/DropdownItem.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ function DropdownItem(props) {
active,
children,
className,
disabled,
description,
icon,
onClick,
Expand All @@ -30,6 +31,7 @@ function DropdownItem(props) {

const classes = cx(
useKeyOnly(active, 'active'),
useKeyOnly(disabled, 'disabled'),
useKeyOnly(selected, 'selected'),
'item',
className,
Expand Down Expand Up @@ -78,6 +80,9 @@ DropdownItem.propTypes = {
/** Additional text with less emphasis. */
description: PropTypes.string,

/** A dropdown item can be disabled. */
disabled: PropTypes.bool,

/** Add an icon to the item. */
icon: PropTypes.string,

Expand Down
56 changes: 56 additions & 0 deletions test/specs/modules/Dropdown/Dropdown-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,22 @@ describe('Dropdown Component', () => {
.simulate('click')
.should.have.prop('selected', true)
})
it('is ignored when clicking a disabled item', () => {
// random item, skip the first as its selected by default
const randomIndex = 1 + _.random(options.length - 2)
const nativeEvent = { nativeEvent: { stopImmediatePropagation: _.noop } }

options[randomIndex].disabled = true

wrapperMount(<Dropdown options={options} selection />)
.simulate('click', nativeEvent)
.find('DropdownItem')
.at(randomIndex)
.simulate('click', nativeEvent)
.should.have.not.prop('selected', true)
Copy link
Member

Choose a reason for hiding this comment

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

Could we reverse the .have.not to .should.not.have.prop('selected', true)?


dropdownMenuIsOpen()
})
it('moves down on arrow down when open', () => {
wrapperMount(<Dropdown options={options} selection />)

Expand Down Expand Up @@ -193,6 +209,29 @@ describe('Dropdown Component', () => {
.find('.selected')
.should.contain.text('a2')
})
it('skips over disabled items', () => {
const opts = [
{ text: 'a1', value: 'a1' },
{ text: 'skip this one', value: 'skip this one', disabled: true },
{ text: 'a2', value: 'a2' },
]
// search for 'a'
wrapperMount(<Dropdown options={opts} search selection />)
.simulate('click')
.find('input.search')
.simulate('change', { target: { value: 'a' } })

wrapper
.find('.selected')
.should.contain.text('a1')

// move selection down
domEvent.keyDown(document, { key: 'ArrowDown' })

wrapper
.find('.selected')
.should.contain.text('a2')
})
it('scrolls the selected item into view', () => {
// get enough options to make the menu scrollable
const opts = getOptions(20)
Expand Down Expand Up @@ -260,6 +299,23 @@ describe('Dropdown Component', () => {

item.should.have.prop('active', true)
})
it('does not become active on enter when disabled', () => {
const disabledOptions = _.map(options, (o) => ({ ...o, disabled: true }))
const item = wrapperMount(<Dropdown options={disabledOptions} selection />)
.simulate('click')
.find('DropdownItem')
.at(0)

// initial item props
item.should.have.prop('selected', true)
Copy link
Member

Choose a reason for hiding this comment

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

Hm, it seems we should never be able to select a disabled item.

item.should.have.prop('active', false)

// attempt to make active
domEvent.keyDown(document, { key: 'Enter' })

item.should.have.prop('active', false)
dropdownMenuIsOpen()
})
it('closes the menu', () => {
wrapperMount(<Dropdown options={options} selection />)
.simulate('click')
Expand Down