Skip to content

Commit

Permalink
SelectDropdown: cleanup code and migrate CSS to webpack (#35086)
Browse files Browse the repository at this point in the history
* SelectDropdown: replace methods bound in constructor with arrow fns

* SelectDropdown: instanceId doesn't need to be in state and be used for keys

Changes `instanceId` to be an instance property assigned during construction. No need
to put it in state, as it never changes.

Also, don't use `instanceId` to create `key` values. These don't need to be globally unique.
Use them only for DOM element IDs that are used to link elements together with ARIA attributes.

* SelectDropdown: remove props argument of getInitialSelectedItem

We can always read from `this.props`. Even in constructor, because we
called `super( props )`.

* SelectDropdown: comment and improve the getInitialSelectedItem logic

Add comments about what the method does, and fix a bug where a separator
item (specified as `null` in `options`) can potentially crash the `find`.

* SelectDropdown: toggle visibility of dropdown with visibility: hidden

Removes need for smart `tabIndex` logic: hidden elements are automatically
excluded from tab order.

`visibility: hidden`, as opposed to `display: none`, still takes the element
into account when doing layout. Therefore, the dropdown header will continue
to have the width of the widest option in the list.

* SelectDropdown: use arrow functions where appropriate

* SelectDropdown: use modern refs for focusing item links

Replace string ref with a modern one, and create a `focusLink` instance method
to provide API for the parent component that wants to set focus. Replace access
to internal `itemRef.refs.itemLink` property.

* SelectDropdown: replace string ref to dropdownContainer with modern one

* SelectDropdown: replace string itemRefs with modern ones

Use an array of refs to focusable items, set them with a callback, move the `refIndex`
increment inline to calling expression.

* SelectDropdown: cloned children don't need a key

`key` is needed only when passing array of children. In this case, the children
are written as JSX and `key` doesn't need to be added on cloning and mapping 1:1.

* SelectDropdown: only DropdownItem children need to be cloned and amended

Only `DropdownItem` needs a ref (it's focusable and the ref is used to move focus
on up and down keyboard navigation) and an `onClick` handler. Separator and Label
are neither focusable nor clickable.

* SelectDropdown: ignore clicks on Label and Separator, add a11y role

Clicks on label and separator should not bubble to the parent element and
cause the dropdown to close. This patch adds a missing handler to the separator
component (label is already OK). Also adds an a11y role to specify that the element
is not interactive despite having an `onClick` handler.

* SelectDropdown: simplify setting initial state

Can be done by assinging an instance property instead of full constructor.
Also, `getInitialSelectedItem` already handles the case where `options` prop
is not present or empty.

* SelectDropdown: remove unneeded componentWillReceiveProps

Closing the popup when receiving new props doesn't seem to make much sense.
And the initial selected value is set only on initial mount and further changes
of the prop are ignored. That's the common behavior of initial-ish props on
uncontrolled components. For example, native `<input defaultValue="x" />` renders
input box with "x" and doesn't change the value on further rerenders with a different
prop.

* SelectDropdown: no need to look at initial selected item in getSelectedText or Icon

In the `getSelectedText` and `getSelectedIcon` getters, the currently selected value
is always in `this.state.selected`. No need to default to the initial value.

Also, use `_.get` instead of `_.result`, as `icon` and `label` are not functions.

* SelectDropdown: remove ESLint-offending and non-helpful JSDoc comment

* SelectDropdown: fix a11y issues by adding roles and correct ARIA attrs

* SelectDropdown: use the classNames helper better

* SelectDropdown: use Lodash noop for event handler defaults

* SelectDropdown: reorg the code that moves focus, use fewer lodash funcs

* SelectDropdown: export Item, Separator and Label as main component fields

* SelectDropdown: rework docs and devdocs example to use the SelectDropdown._ convention

* SelectDropdown: migrate CSS to webpack

* Update usages of SelectDropdown to use the SelectDropdown.Item convention

* Remove import of SelectDropdown stylesheet from EditorPublishData

* SelectDropdown: return undefined from getInitialSelectedItem

* SelectDropdown: update unit tests

Some of them were quite awful, spying on internal method calls or mocking the whole component instance
instead of testing on the real component and checking its state and JSDOM rendering.

* SelectDropdown: disable some ESLint warnings in docs example

* SelectDropdown: hide the dropdown options when the dropdown is closed

Otherwise, the options element is clickable although it has `visibility: hidden`
and captures clicks on controls that are underneath it.
  • Loading branch information
jsnajdr committed Aug 16, 2019
1 parent 09d98af commit da11b17
Show file tree
Hide file tree
Showing 21 changed files with 281 additions and 494 deletions.
1 change: 0 additions & 1 deletion assets/stylesheets/_components.scss
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,5 @@
@import 'components/main/style';
@import 'components/popover/style';
@import 'components/segmented-control/style';
@import 'components/select-dropdown/style';
@import 'components/tooltip/style';
@import 'layout/sidebar/style';
5 changes: 2 additions & 3 deletions client/components/section-nav/tabs.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import { debounce } from 'lodash';
/**
* Internal Dependencies
*/
import DropdownItem from 'components/select-dropdown/item';
import SelectDropdown from 'components/select-dropdown';
import { getWindowInnerWidth } from 'lib/viewport';
import afterLayoutFlush from 'lib/after-layout-flush';
Expand Down Expand Up @@ -120,9 +119,9 @@ class NavTabs extends Component {
return null;
}
return (
<DropdownItem { ...child.props } key={ 'navTabsDropdown-' + index }>
<SelectDropdown.Item { ...child.props } key={ 'navTabsDropdown-' + index }>
{ child.props.children }
</DropdownItem>
</SelectDropdown.Item>
);
} );

Expand Down
34 changes: 16 additions & 18 deletions client/components/select-dropdown/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -16,18 +16,17 @@ A good example for this case is navigation. Sometimes the option that is selecte
```js
import React from 'react';
import SelectDropdown from 'components/select-dropdown';
import DropdownItem from 'components/select-dropdown/item';

export default class extends React.Component {
// ...

render() {
return (
<SelectDropdown selectedText="Published">
<DropdownItem selected={ true } path="/published">Published</DropdownItem>
<DropdownItem path="/scheduled">Scheduled</DropdownItem>
<DropdownItem path="/drafts">Drafts</DropdownItem>
<DropdownItem path="/trashed">Trashed</DropdownItem>
<SelectDropdown.Item selected path="/published">Published</SelectDropdown.Item>
<SelectDropdown.Item path="/scheduled">Scheduled</SelectDropdown.Item>
<SelectDropdown.Item path="/drafts">Drafts</SelectDropdown.Item>
<SelectDropdown.Item path="/trashed">Trashed</SelectDropdown.Item>
</SelectDropdown>
);
}
Expand Down Expand Up @@ -92,24 +91,22 @@ Optional bool to disable dropdown item.

`onClick`

Optional callback that will be applied when a `DropdownItem` has been clicked. This could be used for updating a parent's state, tracking analytics, etc.
Optional callback that will be applied when a `SelectDropdown.Item` has been clicked. This could be used for updating a parent's state, tracking analytics, etc.

### Label

An item "label" can be added like as a sibling to `DropdownItem`. The purpose
of this `DropdownLabel` component is used to display a static item, for example, to group
An item "label" can be added like as a sibling to `SelectDropdown.Item`. The purpose
of this `SelectDropdown.Label` component is used to display a static item, for example, to group
items.

### Separator

As a sibling to `DropdownItem`, an item "separator" or horizontal line can be used to visually separate items.
As a sibling to `SelectDropdown.Item`, an item "separator" or horizontal line can be used to visually separate items.

![separator example screenshot](https://cldup.com/CWEH2K9PUf.png)

```js
import SelectDropdown from 'components/select-dropdown';
import DropdownItem from 'components/select-dropdown/item';
import DropdownSeparator from 'components/select-dropdown/separator';

export default class extends React.Component {

Expand All @@ -118,12 +115,12 @@ export default class extends React.Component {
render() {
return (
<SelectDropdown selectedText="Published">
<DropdownLabel><em>Post status<em></DropdownLabel>
<DropdownItem selected={ true } path="/published">Published</DropdownItem>
<DropdownItem path="/scheduled">Scheduled</DropdownItem>
<DropdownItem path="/drafts">Drafts</DropdownItem>
<DropdownSeparator />
<DropdownItem path="/trashed">Trashed</DropdownItem>
<SelectDropdown.Label><em>Post status<em></SelectDropdown.Label>
<SelectDropdown.Item selected path="/published">Published</SelectDropdown.Item>
<SelectDropdown.Item path="/scheduled">Scheduled</SelectDropdown.Item>
<SelectDropdown.Item path="/drafts">Drafts</SelectDropdown.Item>
<SelectDropdown.Separator />
<SelectDropdown.Item path="/trashed">Trashed</SelectDropdown.Item>
</SelectDropdown>
);
}
Expand All @@ -142,6 +139,7 @@ A good example for this case is a form element. You don't want to have to write
```js
import SelectDropdown from 'components/select-dropdown';

var options = [
{ label: 'Post status', isLabel: true },
{ value: 'published', label: 'Published' },
Expand Down Expand Up @@ -203,7 +201,7 @@ Optional callback that will be run after the dropdown is opened or closed. An ev

### Label

Adding `isLabel` key set to `true` into the item object will create a `DropdownLabel` component.
Adding `isLabel` key set to `true` into the item object will create a `SelectDropdown.Label` component.

```js
var options = [
Expand Down
89 changes: 41 additions & 48 deletions client/components/select-dropdown/docs/example.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,6 @@ import Gridicon from 'gridicons';
* Internal dependencies
*/
import SelectDropdown from 'components/select-dropdown';
import DropdownItem from 'components/select-dropdown/item';
import DropdownLabel from 'components/select-dropdown/label';
import DropdownSeparator from 'components/select-dropdown/separator';

class SelectDropdownExample extends React.PureComponent {
static displayName = 'SelectDropdownExample';
Expand All @@ -29,18 +26,12 @@ class SelectDropdownExample extends React.PureComponent {
],
};

constructor( props ) {
super( props );

const initialState = {
childSelected: 'Published',
selectedCount: 10,
compactButtons: false,
selectedIcon: <Gridicon icon="align-image-left" size={ 18 } />,
};

this.state = initialState;
}
state = {
childSelected: 'Published',
selectedCount: 10,
compactButtons: false,
selectedIcon: <Gridicon icon="align-image-left" size={ 18 } />,
};

toggleButtons = () => {
this.setState( { compactButtons: ! this.state.compactButtons } );
Expand All @@ -51,9 +42,9 @@ class SelectDropdownExample extends React.PureComponent {

return (
<div className="docs__select-dropdown-container">
<a className="docs__design-toggle button" onClick={ this.toggleButtons }>
<button className="docs__design-toggle button" onClick={ this.toggleButtons }>
{ toggleButtonsText }
</a>
</button>

<h3>Items passed as options prop</h3>
<SelectDropdown
Expand All @@ -69,43 +60,43 @@ class SelectDropdownExample extends React.PureComponent {
selectedText={ this.state.childSelected }
selectedCount={ this.state.selectedCount }
>
<DropdownLabel>
<SelectDropdown.Label>
<strong>Statuses</strong>
</DropdownLabel>
</SelectDropdown.Label>

<DropdownItem
<SelectDropdown.Item
count={ 10 }
selected={ this.state.childSelected === 'Published' }
onClick={ this.getSelectItemHandler( 'Published', 10 ) }
>
Published
</DropdownItem>
</SelectDropdown.Item>

<DropdownItem
<SelectDropdown.Item
count={ 4 }
selected={ this.state.childSelected === 'Scheduled' }
onClick={ this.getSelectItemHandler( 'Scheduled', 4 ) }
>
Scheduled
</DropdownItem>
</SelectDropdown.Item>

<DropdownItem
<SelectDropdown.Item
count={ 3343 }
selected={ this.state.childSelected === 'Drafts' }
onClick={ this.getSelectItemHandler( 'Drafts', 3343 ) }
>
Drafts
</DropdownItem>
</SelectDropdown.Item>

<DropdownSeparator />
<SelectDropdown.Separator />

<DropdownItem
<SelectDropdown.Item
count={ 3 }
selected={ this.state.childSelected === 'Trashed' }
onClick={ this.getSelectItemHandler( 'Trashed', 3 ) }
>
Trashed
</DropdownItem>
</SelectDropdown.Item>
</SelectDropdown>

<h3 style={ { marginTop: 20 } }>With Icons in Items Passed as Options</h3>
Expand Down Expand Up @@ -143,11 +134,11 @@ class SelectDropdownExample extends React.PureComponent {
selectedText={ this.state.childSelected }
selectedIcon={ this.state.selectedIcon }
>
<DropdownLabel>
<SelectDropdown.Label>
<strong>Statuses</strong>
</DropdownLabel>
</SelectDropdown.Label>

<DropdownItem
<SelectDropdown.Item
selected={ this.state.childSelected === 'Published' }
icon={ <Gridicon icon="align-image-left" size={ 18 } /> }
onClick={ this.getSelectItemHandler(
Expand All @@ -157,9 +148,9 @@ class SelectDropdownExample extends React.PureComponent {
) }
>
Published
</DropdownItem>
</SelectDropdown.Item>

<DropdownItem
<SelectDropdown.Item
selected={ this.state.childSelected === 'Scheduled' }
icon={ <Gridicon icon="calendar" size={ 18 } /> }
onClick={ this.getSelectItemHandler(
Expand All @@ -169,9 +160,9 @@ class SelectDropdownExample extends React.PureComponent {
) }
>
Scheduled
</DropdownItem>
</SelectDropdown.Item>

<DropdownItem
<SelectDropdown.Item
selected={ this.state.childSelected === 'Drafts' }
icon={ <Gridicon icon="create" size={ 18 } /> }
onClick={ this.getSelectItemHandler(
Expand All @@ -181,11 +172,11 @@ class SelectDropdownExample extends React.PureComponent {
) }
>
Drafts
</DropdownItem>
</SelectDropdown.Item>

<DropdownSeparator />
<SelectDropdown.Separator />

<DropdownItem
<SelectDropdown.Item
selected={ this.state.childSelected === 'Trashed' }
icon={ <Gridicon icon="trash" size={ 18 } /> }
onClick={ this.getSelectItemHandler(
Expand All @@ -195,7 +186,7 @@ class SelectDropdownExample extends React.PureComponent {
) }
>
Trashed
</DropdownItem>
</SelectDropdown.Item>
</SelectDropdown>

<h3 style={ { marginTop: 20 } }>max-width: 220px;</h3>
Expand All @@ -206,17 +197,17 @@ class SelectDropdownExample extends React.PureComponent {
selectedText="Published publish publish publish"
selectedCount={ 10 }
>
<DropdownLabel>
<SelectDropdown.Label>
<strong>Statuses</strong>
</DropdownLabel>
<DropdownItem count={ 10 } selected={ true }>
</SelectDropdown.Label>
<SelectDropdown.Item count={ 10 } selected={ true }>
Published publish publish publish
</DropdownItem>
<DropdownItem count={ 4 }> Scheduled scheduled</DropdownItem>
<DropdownItem count={ 3343 }>Drafts</DropdownItem>
<DropdownItem disabled={ true }>Disabled Item</DropdownItem>
<DropdownSeparator />
<DropdownItem count={ 3 }>Trashed</DropdownItem>
</SelectDropdown.Item>
<SelectDropdown.Item count={ 4 }> Scheduled scheduled</SelectDropdown.Item>
<SelectDropdown.Item count={ 3343 }>Drafts</SelectDropdown.Item>
<SelectDropdown.Item disabled={ true }>Disabled Item</SelectDropdown.Item>
<SelectDropdown.Separator />
<SelectDropdown.Item count={ 3 }>Trashed</SelectDropdown.Item>
</SelectDropdown>

<h3 style={ { marginTop: 20 } }>Disabled State</h3>
Expand Down Expand Up @@ -244,10 +235,12 @@ class SelectDropdownExample extends React.PureComponent {
selectedIcon: icon,
} );

// eslint-disable-next-line no-console
console.log( 'Select Dropdown Item (selected):', childSelected );
};

onDropdownSelect( option ) {
// eslint-disable-next-line no-console
console.log( 'Select Dropdown (selected):', option );
}
}
Expand Down
Loading

0 comments on commit da11b17

Please sign in to comment.