Skip to content

Commit

Permalink
refactor: use string based selection in multi- and single-select
Browse files Browse the repository at this point in the history
  • Loading branch information
HendrikThePendric authored Apr 1, 2020
2 parents a4db70e + e3627a4 commit 95600e4
Show file tree
Hide file tree
Showing 37 changed files with 188 additions and 220 deletions.
7 changes: 2 additions & 5 deletions cypress/integration/MultiSelect/allows_selecting/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ Then('the clicked option is selected', () => {
cy.window().then(win => {
expect(win.onChange).to.be.calledOnce
expect(win.onChange).to.be.calledWith({
selected: [{ label: 'option one', value: '1' }],
selected: ['1'],
})
})
})
Expand All @@ -50,10 +50,7 @@ Then('the clicked option is selected as well', () => {
cy.window().then(win => {
expect(win.onChange).to.be.calledOnce
expect(win.onChange).to.be.calledWith({
selected: [
{ label: 'option one', value: '1' },
{ label: 'option two', value: '2' },
],
selected: ['1', '2'],
})
})
})
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
Feature: Duplicate option values result in a conflict between the input and dropdown UI

Scenario: The MultiSelect has options with a duplicate value and this value is selected
Given a MultiSelect with options with a duplicate value and this value is selected
And the MultiSelect is open
Then the first option with the selected value is displayed in the input
But both options are highlighted in the dropdown
22 changes: 22 additions & 0 deletions cypress/integration/MultiSelect/duplicate_option_values/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
import '../common'
import { Given, Then } from 'cypress-cucumber-preprocessor/steps'

Given(
'a MultiSelect with options with a duplicate value and this value is selected',
() => {
cy.visitStory('MultiSelect', 'With duplicate selected option values')
}
)
Then(
'the first option with the selected value is displayed in the input',
() => {
cy.get('[data-test="dhis2-uicore-select-input"]').should(
'contain',
'option one'
)
}
)
Then('both options are highlighted in the dropdown', () => {
cy.get('input[name="option one"]').should('have.attr', 'checked')
cy.get('input[name="option one a"]').should('have.attr', 'checked')
})
2 changes: 1 addition & 1 deletion cypress/integration/SingleSelect/accepts_blur_cb/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ Then('the onBlur handler is called', () => {
cy.window().then(win => {
expect(win.onBlur).to.be.calledOnce
expect(win.onBlur).to.be.calledWith({
selected: {},
selected: '',
})
})
})
2 changes: 1 addition & 1 deletion cypress/integration/SingleSelect/accepts_focus_cb/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ Then('the onFocus handler is called', () => {
cy.window().then(win => {
expect(win.onFocus).to.be.calledOnce
expect(win.onFocus).to.be.calledWith({
selected: {},
selected: '',
})
})
})
2 changes: 1 addition & 1 deletion cypress/integration/SingleSelect/allows_selecting/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ Then('the clicked option is selected', () => {
cy.window().then(win => {
expect(win.onChange).to.be.calledOnce
expect(win.onChange).to.be.calledWith({
selected: { label: 'option one', value: '1' },
selected: '1',
})
})
})
Expand Down
2 changes: 1 addition & 1 deletion cypress/integration/SingleSelect/can_be_cleared/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,6 @@ When('the clear button is clicked', () => {
Then('the SingleSelect is cleared', () => {
cy.window().then(win => {
expect(win.onChange).to.be.calledOnce
expect(win.onChange).to.be.calledWith({ selected: {} })
expect(win.onChange).to.be.calledWith({ selected: '' })
})
})
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
Feature: Duplicate option values result in a conflict between the input and dropdown UI

Scenario: The SingleSelect has options with a duplicate value and this value is selected
Given a SingleSelect with options with a duplicate value and this value is selected
And the SingleSelect is open
Then the first option with the selected value is displayed in the input
But both options are highlighted in the dropdown
27 changes: 27 additions & 0 deletions cypress/integration/SingleSelect/duplicate_option_values/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
import '../common'
import { Given, Then } from 'cypress-cucumber-preprocessor/steps'

Given(
'a SingleSelect with options with a duplicate value and this value is selected',
() => {
cy.visitStory('SingleSelect', 'With duplicate selected option values')
}
)
Then(
'the first option with the selected value is displayed in the input',
() => {
cy.get('[data-test="dhis2-uicore-select-input"]').should(
'contain',
'option one'
)
}
)
Then('both options are highlighted in the dropdown', () => {
cy.get('[data-test="dhis2-uicore-singleselectoption"]')
.contains('option one')
.should('have.class', 'active')

cy.get('[data-test="dhis2-uicore-singleselectoption"]')
.contains('option one a')
.should('have.class', 'active')
})
11 changes: 4 additions & 7 deletions cypress/integration/forms/MultiSelect/can_set_a_value/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,20 +27,17 @@ When('the user selects the second option', () => {
})

Then("the form state's value equals the first option's value", () => {
cy.get('@options').then(options => {
const [firstOption] = options
cy.getFormValue('multiSelect').then(selected => {
expect(selected).to.have.lengthOf(1)
expect(selected).to.deep.equal([firstOption])
})
cy.getFormValue('multiSelect').then(selected => {
expect(selected).to.have.lengthOf(1)
expect(selected).to.deep.equal(['value1'])
})
})

Then("the form state's value contains both options", () => {
cy.get('@options').then(options => {
cy.getFormValue('multiSelect').then(selected => {
expect(selected).to.have.lengthOf(options.length)
expect(selected).to.deep.equal(options)
expect(selected).to.deep.equal(['value1', 'value2'])
})
})
})
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ When('the user selects the first option', () => {
Then("the form state's value equals the first option's value", () => {
cy.get('@options').then(options => {
cy.getFormValue('singleSelect').then(actualValue => {
expect(actualValue).to.deep.equal(options[0])
expect(actualValue).to.deep.equal(options[0].value)
})
})
})
15 changes: 0 additions & 15 deletions packages/constants/src/shared-prop-types.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,21 +35,6 @@ export const sizePropType = propTypes.mutuallyExclusive(
propTypes.bool
)

/**
* SingleSelect selected item propType
* @return {PropType} Object with props `label` and `value`.
*/
export const singleSelectedPropType = propTypes.shape({
label: propTypes.string,
value: propTypes.string,
})

/**
* SingleSelect selected item propType
* @return {Array} Array of Objects with props `label` and `value`.
*/
export const multiSelectedPropType = propTypes.arrayOf(singleSelectedPropType)

/**
* Inside alignment props
* @return {PropType} PropType that validates the inside alignment.
Expand Down
7 changes: 6 additions & 1 deletion packages/forms/src/MultiSelectControl/MultiSelectControl.js
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,12 @@ MultiSelectControl.propTypes = {

error: propTypes.bool,
loading: propTypes.bool,
options: MultiSelectField.propTypes.selected,
options: propTypes.arrayOf(
propTypes.shape({
label: propTypes.string,
value: propTypes.string,
})
),
showLoadingStatus: propTypes.bool,
showValidStatus: propTypes.bool,
valid: propTypes.bool,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,7 @@ const options = [
{ value: '10', label: 'ten' },
]

const initialValue = [
{ value: '3', label: 'three' },
{ value: '4', label: 'four' },
{ value: '9', label: 'nine' },
{ value: '10', label: 'ten' },
]
const initialValue = ['3', '4', '9', '10']

storiesOf('Form/MultiSelectControl', module)
.addDecorator(formDecorator)
Expand Down
11 changes: 7 additions & 4 deletions packages/forms/src/SingleSelectControl/SingleSelectControl.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,7 @@ export const SingleSelectControl = ({
onFocus={createFocusHandler(input, onFocus)}
onChange={createSelectChangeHandler(input)}
onBlur={createBlurHandler(input, onBlur)}
selected={
input.value || {}
} /* input.value is an empty string initially, so we're providing an empty object if falsey */
selected={input.value || ''}
>
{options.map(option => (
<SingleSelectOption key={option.value} {...option} />
Expand All @@ -53,7 +51,12 @@ export const SingleSelectControl = ({
SingleSelectControl.propTypes = {
input: inputPropType.isRequired,
meta: metaPropType.isRequired,
options: propTypes.arrayOf(SingleSelectField.propTypes.selected).isRequired,
options: propTypes.arrayOf(
propTypes.shape({
label: propTypes.string,
value: propTypes.string,
})
).isRequired,

error: propTypes.bool,
loading: propTypes.bool,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,6 @@ const options = [
{ value: '10', label: 'ten' },
]

const initialValue = { value: '4', label: 'four' }

storiesOf('Form/SingleSelectControl', module)
.addDecorator(formDecorator)
.add('Default', () => (
Expand All @@ -35,6 +33,6 @@ storiesOf('Form/SingleSelectControl', module)
name="agree"
label="Do you agree?"
options={options}
initialValue={initialValue}
initialValue="4"
/>
))
3 changes: 1 addition & 2 deletions packages/widgets/src/MultiSelect/FilterableMenu.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import React from 'react'
import propTypes from '@dhis2/prop-types'
import { sharedPropTypes } from '@dhis2/ui-constants'
import { FilterableMenu as CommonFilterableMenu } from '../Select/FilterableMenu.js'
import { Menu } from './Menu.js'

Expand Down Expand Up @@ -37,7 +36,7 @@ FilterableMenu.propTypes = {
handleFocusInput: propTypes.func,
options: propTypes.node,
placeholder: propTypes.string,
selected: sharedPropTypes.multiSelectedPropType,
selected: propTypes.arrayOf(propTypes.string),
onChange: propTypes.func,
}

Expand Down
4 changes: 2 additions & 2 deletions packages/widgets/src/MultiSelect/Input.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import React from 'react'
import propTypes from '@dhis2/prop-types'
import cx from 'classnames'
import { colors, sharedPropTypes } from '@dhis2/ui-constants'
import { colors } from '@dhis2/ui-constants'
import { SelectionList } from './SelectionList.js'
import { InputPlaceholder } from '../Select/InputPlaceholder.js'
import { InputPrefix } from '../Select/InputPrefix.js'
Expand Down Expand Up @@ -101,7 +101,7 @@ Input.propTypes = {
options: propTypes.node,
placeholder: propTypes.string,
prefix: propTypes.string,
selected: sharedPropTypes.multiSelectedPropType,
selected: propTypes.arrayOf(propTypes.string),
onChange: propTypes.func,
}

Expand Down
15 changes: 5 additions & 10 deletions packages/widgets/src/MultiSelect/Menu.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import React from 'react'
import propTypes from '@dhis2/prop-types'
import { sharedPropTypes } from '@dhis2/ui-constants'
import { Empty } from '../Select/Empty.js'
import {
filterIgnored,
Expand All @@ -14,25 +13,21 @@ const onDisabledClick = (_, e) => {
e.preventDefault()
}

const createHandler = ({ isActive, onChange, selected, value, label }) => (
_,
e
) => {
const clickedOption = { value, label }
const createHandler = ({ isActive, onChange, selected, value }) => (_, e) => {
e.stopPropagation()
e.preventDefault()

// If the option is currently selected remove it from the array of selected options
if (isActive) {
const filtered = removeOption(clickedOption, selected)
const filtered = removeOption(value, selected)
const data = { selected: filtered }

return onChange(data, e)
}

// Otherwise, add it to selected
const data = {
selected: selected.concat([clickedOption]),
selected: selected.concat([value]),
}
return onChange(data, e)
}
Expand Down Expand Up @@ -61,7 +56,7 @@ const Menu = ({ options, onChange, selected, empty, dataTest }) => {
const { value, label, disabled: isDisabled } = child.props

// Active means the option is currently selected
const isActive = !!findOption({ value, label }, selected)
const isActive = !!findOption(value, selected)

// Create the appropriate click handler for the option
const onClick = isDisabled
Expand Down Expand Up @@ -92,7 +87,7 @@ Menu.propTypes = {
dataTest: propTypes.string.isRequired,
empty: propTypes.node,
options: propTypes.node,
selected: sharedPropTypes.multiSelectedPropType,
selected: propTypes.arrayOf(propTypes.string),
onChange: propTypes.func,
}

Expand Down
4 changes: 2 additions & 2 deletions packages/widgets/src/MultiSelect/MultiSelect.js
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ MultiSelect.defaultProps = {
* @static
*
* @prop {function} [onChange]
* @prop {Array} [selected]
* @prop {Array.<string>} [selected]
* @prop {string} [className]
* @prop {string} [tabIndex]
* @prop {Node} [children]
Expand Down Expand Up @@ -175,7 +175,7 @@ MultiSelect.propTypes = {
),
placeholder: propTypes.string,
prefix: propTypes.string,
selected: sharedPropTypes.multiSelectedPropType,
selected: propTypes.arrayOf(propTypes.string),
tabIndex: propTypes.string,
valid: sharedPropTypes.statusPropType,
warning: sharedPropTypes.statusPropType,
Expand Down
Loading

0 comments on commit 95600e4

Please sign in to comment.