From 1f442612796ef93a948708d56c038573a796fa10 Mon Sep 17 00:00:00 2001 From: Chandler Prall Date: Mon, 14 May 2018 15:48:28 -0600 Subject: [PATCH 1/7] Move EuiBasicTable's itemId prop out of selection into a top-level prop --- src-docs/src/views/tables/actions/actions.js | 2 +- src-docs/src/views/tables/basic/props_info.js | 10 +++++----- .../tables/expanding_rows/expanding_rows.js | 2 +- .../tables/in_memory/in_memory_selection.js | 2 +- src-docs/src/views/tables/mobile/mobile.js | 2 +- src-docs/src/views/tables/selection/selection.js | 2 +- .../__snapshots__/in_memory_table.test.js.snap | 12 ++++++------ .../basic_table/basic_table.behavior.test.js | 2 +- src/components/basic_table/basic_table.js | 16 ++++++++-------- src/components/basic_table/basic_table.test.js | 14 +++++++------- src/components/basic_table/in_memory_table.js | 6 +++++- .../basic_table/in_memory_table.test.js | 12 ++++++------ 12 files changed, 43 insertions(+), 39 deletions(-) diff --git a/src-docs/src/views/tables/actions/actions.js b/src-docs/src/views/tables/actions/actions.js index c622eea21cd..f4d868f2126 100644 --- a/src-docs/src/views/tables/actions/actions.js +++ b/src-docs/src/views/tables/actions/actions.js @@ -262,7 +262,6 @@ export class Table extends Component { }; const selection = { - itemId: 'id', selectable: (user) => user.online, selectableMessage: (selectable) => !selectable ? 'User is currently offline' : undefined, onSelectionChange: this.onSelectionChange @@ -292,6 +291,7 @@ export class Table extends Component { string' } + }, compressed: { description: 'Makes the font and padding smaller for the entire table', type: { name: 'bool' } @@ -88,11 +93,6 @@ export const propsInfo = { __docgenInfo: { _euiObjectType: 'type', props: { - itemId: { - description: 'describes how to extract a unique ID from each item', - required: true, - type: { name: 'string | (item) => string' } - }, onSelectionChanged: { description: 'A callback that will be called whenever the item selection changes', required: false, diff --git a/src-docs/src/views/tables/expanding_rows/expanding_rows.js b/src-docs/src/views/tables/expanding_rows/expanding_rows.js index fcae31a3546..5dd89ee47c9 100644 --- a/src-docs/src/views/tables/expanding_rows/expanding_rows.js +++ b/src-docs/src/views/tables/expanding_rows/expanding_rows.js @@ -208,7 +208,6 @@ export class Table extends Component { }; const selection = { - itemId: 'id', selectable: (user) => user.online, selectableMessage: (selectable) => !selectable ? 'User is currently offline' : undefined, onSelectionChange: this.onSelectionChange @@ -219,6 +218,7 @@ export class Table extends Component { {deleteButton} user.online, selectableMessage: (selectable) => !selectable ? 'User is currently offline' : undefined, onSelectionChange: (selection) => this.setState({ selection }) @@ -227,6 +226,7 @@ export class Table extends Component {
user.online, selectableMessage: (selectable) => !selectable ? 'User is currently offline' : undefined, onSelectionChange: this.onSelectionChange @@ -219,6 +218,7 @@ export class Table extends Component { user.online, selectableMessage: (selectable) => !selectable ? 'User is currently offline' : undefined, onSelectionChange: this.onSelectionChange @@ -203,6 +202,7 @@ export class Table extends Component { {deleteButton} { { id: '1', name: 'name1' }, { id: '2', name: 'name2' } ], + itemId: 'id', columns: [ { field: 'name', @@ -24,7 +25,6 @@ describe('EuiBasicTable', () => { } ], selection: { - itemId: 'id', onSelectionChanged: () => {} }, onChange: () => {} diff --git a/src/components/basic_table/basic_table.js b/src/components/basic_table/basic_table.js index 3e522a5fd08..356a30eba42 100644 --- a/src/components/basic_table/basic_table.js +++ b/src/components/basic_table/basic_table.js @@ -111,13 +111,12 @@ export const ComputedColumnType = PropTypes.shape({ export const ColumnType = PropTypes.oneOfType([FieldDataColumnType, ComputedColumnType, ActionsColumnType]); -const ItemIdType = PropTypes.oneOfType([ +export const ItemIdType = PropTypes.oneOfType([ PropTypes.string, // the name of the item id property PropTypes.func // (item) => string ]); export const SelectionType = PropTypes.shape({ - itemId: ItemIdType.isRequired, onSelectionChange: PropTypes.func, // (selection: Record[]) => void;, selectable: PropTypes.func, // (item) => boolean; selectableMessage: PropTypes.func // (selectable, item) => boolean; @@ -129,6 +128,7 @@ const SortingType = PropTypes.shape({ const BasicTablePropTypes = { items: PropTypes.array.isRequired, + itemId: ItemIdType, columns: PropTypes.arrayOf(ColumnType).isRequired, pagination: PaginationType, sorting: SortingType, @@ -172,12 +172,12 @@ export class EuiBasicTable extends Component { } itemId(item) { - const { selection } = this.props; - if (selection) { - if (isFunction(selection.itemId)) { - return selection.itemId(item); + const { itemId } = this.props; + if (itemId) { + if (isFunction(itemId)) { + return itemId(item); } - return item[selection.itemId]; + return item[itemId]; } } @@ -480,7 +480,7 @@ export class EuiBasicTable extends Component { const cells = []; - const itemId = selection ? this.itemId(item) : rowIndex; + const itemId = this.itemId(item) || rowIndex; const selected = !selection ? false : this.state.selection && !!this.state.selection.find(selectedRecord => ( this.itemId(selectedRecord) === itemId )); diff --git a/src/components/basic_table/basic_table.test.js b/src/components/basic_table/basic_table.test.js index 53e27695f96..0aeac14bb8d 100644 --- a/src/components/basic_table/basic_table.test.js +++ b/src/components/basic_table/basic_table.test.js @@ -272,6 +272,7 @@ describe('EuiBasicTable', () => { { id: '2', name: 'name2' }, { id: '3', name: 'name3' } ], + itemId: 'id', columns: [ { field: 'name', @@ -285,7 +286,6 @@ describe('EuiBasicTable', () => { totalItemCount: 5 }, selection: { - itemId: 'id', onSelectionChanged: () => undefined }, onChange: () => {} @@ -306,6 +306,7 @@ describe('EuiBasicTable', () => { { id: '2', name: 'name2' }, { id: '3', name: 'name3' } ], + itemId: 'id', columns: [ { field: 'name', @@ -320,7 +321,6 @@ describe('EuiBasicTable', () => { totalItemCount: 5 }, selection: { - itemId: 'id', onSelectionChanged: () => undefined }, sorting: { @@ -344,6 +344,7 @@ describe('EuiBasicTable', () => { { id: '2', name: 'name2' }, { id: '3', name: 'name3' } ], + itemId: 'id', columns: [ { field: 'name', @@ -359,7 +360,6 @@ describe('EuiBasicTable', () => { totalItemCount: 5 }, selection: { - itemId: 'id', onSelectionChanged: () => undefined }, sorting: { @@ -383,6 +383,7 @@ describe('EuiBasicTable', () => { { id: '2', count: 2 }, { id: '3', count: 3 } ], + itemId: 'id', columns: [ { field: 'count', @@ -398,7 +399,6 @@ describe('EuiBasicTable', () => { totalItemCount: 5 }, selection: { - itemId: 'id', onSelectionChanged: () => undefined }, sorting: { @@ -423,6 +423,7 @@ describe('EuiBasicTable', () => { { id: '2', count: 2 }, { id: '3', count: 3 } ], + itemId: 'id', columns: [ { field: 'count', @@ -439,7 +440,6 @@ describe('EuiBasicTable', () => { totalItemCount: 5 }, selection: { - itemId: 'id', onSelectionChanged: () => undefined }, sorting: { @@ -463,6 +463,7 @@ describe('EuiBasicTable', () => { { id: '2', name: 'name2' }, { id: '3', name: 'name3' } ], + itemId: 'id', columns: [ { field: 'name', @@ -488,7 +489,6 @@ describe('EuiBasicTable', () => { totalItemCount: 5 }, selection: { - itemId: 'id', onSelectionChanged: () => undefined }, sorting: { @@ -512,6 +512,7 @@ describe('EuiBasicTable', () => { { id: '2', name: 'name2' }, { id: '3', name: 'name3' } ], + itemId: 'id', columns: [ { field: 'name', @@ -543,7 +544,6 @@ describe('EuiBasicTable', () => { totalItemCount: 5 }, selection: { - itemId: 'id', onSelectionChanged: () => undefined }, sorting: { diff --git a/src/components/basic_table/in_memory_table.js b/src/components/basic_table/in_memory_table.js index d06bf0f05da..ba851cac69d 100644 --- a/src/components/basic_table/in_memory_table.js +++ b/src/components/basic_table/in_memory_table.js @@ -4,6 +4,7 @@ import { EuiBasicTable, ColumnType, SelectionType, + ItemIdType, } from './basic_table'; import { defaults as paginationBarDefaults @@ -54,7 +55,8 @@ const InMemoryTablePropTypes = { sort: PropertySortType }) ]), - selection: SelectionType + selection: SelectionType, + itemId: ItemIdType }; const getInitialQuery = (search) => { @@ -260,6 +262,7 @@ export class EuiInMemoryTable extends Component { pagination: hasPagination, sorting: hasSorting, itemIdToExpandedRowMap, + itemId, } = this.props; const { @@ -295,6 +298,7 @@ export class EuiInMemoryTable extends Component { const table = ( { { id: '2', name: 'name2' }, { id: '3', name: 'name3' } ], + itemId: 'id', columns: [ { field: 'name', @@ -302,7 +303,6 @@ describe('EuiInMemoryTable', () => { ], pagination: true, selection: { - itemId: 'id', onSelectionChanged: () => undefined } }; @@ -322,6 +322,7 @@ describe('EuiInMemoryTable', () => { { id: '2', name: 'name2' }, { id: '3', name: 'name3' } ], + itemId: 'id', columns: [ { field: 'name', @@ -333,7 +334,6 @@ describe('EuiInMemoryTable', () => { pagination: true, sorting: true, selection: { - itemId: 'id', onSelectionChanged: () => undefined } }; @@ -353,6 +353,7 @@ describe('EuiInMemoryTable', () => { { id: '2', name: 'name2' }, { id: '3', name: 'name3' } ], + itemId: 'id', columns: [ { field: 'name', @@ -367,7 +368,6 @@ describe('EuiInMemoryTable', () => { }, sorting: true, selection: { - itemId: 'id', onSelectionChanged: () => undefined } }; @@ -387,6 +387,7 @@ describe('EuiInMemoryTable', () => { { id: '2', name: 'name2' }, { id: '3', name: 'name3' } ], + itemId: 'id', columns: [ { field: 'name', @@ -409,7 +410,6 @@ describe('EuiInMemoryTable', () => { pagination: true, sorting: true, selection: { - itemId: 'id', onSelectionChanged: () => undefined } }; @@ -429,6 +429,7 @@ describe('EuiInMemoryTable', () => { { id: '2', name: 'name2' }, { id: '3', name: 'name3' } ], + itemId: 'id', columns: [ { field: 'name', @@ -452,7 +453,6 @@ describe('EuiInMemoryTable', () => { sorting: true, search: true, selection: { - itemId: 'id', onSelectionChanged: () => undefined } }; @@ -472,6 +472,7 @@ describe('EuiInMemoryTable', () => { { id: '2', name: 'name2' }, { id: '3', name: 'name3' } ], + itemId: 'id', columns: [ { field: 'name', @@ -510,7 +511,6 @@ describe('EuiInMemoryTable', () => { ] }, selection: { - itemId: 'id', onSelectionChanged: () => undefined } }; From 41c67ef55448d551d19937e329768ca4c36e6693 Mon Sep 17 00:00:00 2001 From: Chandler Prall Date: Mon, 14 May 2018 16:03:37 -0600 Subject: [PATCH 2/7] changelog --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 05517fe0fc3..7b22bc542a3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,10 @@ - Made boolean matching in `EuiSearchBar` more exact so it doesn't match words starting with booleans, like "truest" or "offer" ([#776](https://github.com/elastic/eui/pull/776)) - `EuiComboBox` do not setState or call refs once component is unmounted ([807](https://github.com/elastic/eui/pull/807) and [#813](https://github.com/elastic/eui/pull/813)) +**Breaking changes** + +- Move `EuiBasicTable`'s `itemId` prop from `selection` to a top-level property [#830](https://github.com/elastic/eui/pull/830) + ## [`0.0.46`](https://github.com/elastic/eui/tree/v0.0.46) - Added `EuiDescribedFormGroup` component, a wrapper around `EuiFormRow`(s) ([#707](https://github.com/elastic/eui/pull/707)) From 63a4b0d9fd5a56c8c6c245789652bdf1cfe564ea Mon Sep 17 00:00:00 2001 From: Chandler Prall Date: Tue, 15 May 2018 10:53:33 -0600 Subject: [PATCH 3/7] add generic proptype validator for associating properties --- CHANGELOG.md | 4 +-- src/components/badge/badge.js | 19 +++++----- .../__snapshots__/basic_table.test.js.snap | 34 +++++++++--------- .../in_memory_table.test.js.snap | 17 +-------- src/components/basic_table/basic_table.js | 7 ++-- .../basic_table/basic_table.test.js | 1 + .../basic_table/in_memory_table.test.js | 1 + src/utils/prop_types/index.js | 4 +-- src/utils/prop_types/requires_aria_label.js | 27 -------------- src/utils/prop_types/with_required_prop.js | 35 +++++++++++++++++++ 10 files changed, 74 insertions(+), 75 deletions(-) delete mode 100644 src/utils/prop_types/requires_aria_label.js create mode 100644 src/utils/prop_types/with_required_prop.js diff --git a/CHANGELOG.md b/CHANGELOG.md index 7b22bc542a3..c786c479cf1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,14 +9,14 @@ - Fixes height calculation error on `EuiAccordion` when it starts loads in an open state. ([#816](https://github.com/elastic/eui/pull/816)) - Added aria-invalid labeling on `EuiFormRow` ([#777](https://github.com/elastic/eui/pull/799)) - Added aria-live labeling for `EuiToasts` ([#777](https://github.com/elastic/eui/pull/777)) -- Added aria labeling requirements for `EuiBadge` , as well as a generic prop_type function `requiresAriaLabel` in `utils` to check for it. ([#777](https://github.com/elastic/eui/pull/777)) ([#802](https://github.com/elastic/eui/pull/802)) +- Added aria labeling requirements for `EuiBadge` , as well as a generic prop_type function `withRequiredProp` in `utils` to check for it. ([#777](https://github.com/elastic/eui/pull/777)) ([#802](https://github.com/elastic/eui/pull/802)) - Ensure switches’ inputs are still hidden when `[disabled]` ([#778](https://github.com/elastic/eui/pull/778)) - Made boolean matching in `EuiSearchBar` more exact so it doesn't match words starting with booleans, like "truest" or "offer" ([#776](https://github.com/elastic/eui/pull/776)) - `EuiComboBox` do not setState or call refs once component is unmounted ([807](https://github.com/elastic/eui/pull/807) and [#813](https://github.com/elastic/eui/pull/813)) **Breaking changes** -- Move `EuiBasicTable`'s `itemId` prop from `selection` to a top-level property [#830](https://github.com/elastic/eui/pull/830) +- Move `EuiBasicTable`'s `itemId` prop from `selection` to a top-level property ([#830](https://github.com/elastic/eui/pull/830)) ## [`0.0.46`](https://github.com/elastic/eui/tree/v0.0.46) diff --git a/src/components/badge/badge.js b/src/components/badge/badge.js index fce948ada9b..8d527c80219 100644 --- a/src/components/badge/badge.js +++ b/src/components/badge/badge.js @@ -155,22 +155,25 @@ EuiBadge.propTypes = { /** * Will apply an onclick to icon within the badge */ - iconOnClick: PropTypes.func, + iconOnClick: EuiPropTypes.withRequiredProp( + PropTypes.func, + 'iconOnClickAriaLabel', + 'Please provide an aria label to compliment your iconOnClick' + ), /** * Aria label applied to the iconOnClick button */ - iconOnClickAriaLabel: EuiPropTypes.requiresAriaLabel('iconOnClick'), + iconOnClickAriaLabel: PropTypes.string, /** * Will apply an onclick to the badge itself */ - onClick: PropTypes.func, - - /** - * Aria label applied to the onClick button - */ - onClickAriaLabel: EuiPropTypes.requiresAriaLabel('onClick'), + onClick: EuiPropTypes.withRequiredProp( + PropTypes.func, + 'onClickAriaLabel', + 'Please provide an aria label to compliment your onClick' + ), /** * Accepts either our palette colors (primary, secondary ..etc) or a hex value `#FFFFFF`, `#000`. diff --git a/src/components/basic_table/__snapshots__/basic_table.test.js.snap b/src/components/basic_table/__snapshots__/basic_table.test.js.snap index f8b9fb5ca9c..7db2d527140 100644 --- a/src/components/basic_table/__snapshots__/basic_table.test.js.snap +++ b/src/components/basic_table/__snapshots__/basic_table.test.js.snap @@ -205,22 +205,6 @@ exports[`EuiBasicTable itemIdToExpandedRowMap renders an expanded row 1`] = ` - - - - name1 - - - @@ -234,7 +218,7 @@ exports[`EuiBasicTable itemIdToExpandedRowMap renders an expanded row 1`] = ` key="_data_column_name_1_0" textOnly={true} > - name2 + name1 + name2 + + + + + + name3 diff --git a/src/components/basic_table/__snapshots__/in_memory_table.test.js.snap b/src/components/basic_table/__snapshots__/in_memory_table.test.js.snap index 792dbc22b35..5a5e03bfbe2 100644 --- a/src/components/basic_table/__snapshots__/in_memory_table.test.js.snap +++ b/src/components/basic_table/__snapshots__/in_memory_table.test.js.snap @@ -11,7 +11,6 @@ exports[`EuiInMemoryTable empty array 1`] = ` }, ] } - itemIdToExpandedRowMap={Object {}} items={Array []} noItemsMessage="No items found" onChange={[Function]} @@ -31,7 +30,6 @@ exports[`EuiInMemoryTable with initial sorting 1`] = ` }, ] } - itemIdToExpandedRowMap={Object {}} items={ Array [ Object { @@ -73,7 +71,6 @@ exports[`EuiInMemoryTable with items 1`] = ` }, ] } - itemIdToExpandedRowMap={Object {}} items={ Array [ Object { @@ -107,6 +104,7 @@ exports[`EuiInMemoryTable with items and expanded item 1`] = ` }, ] } + itemId="id" itemIdToExpandedRowMap={ Object { "1":
@@ -147,7 +145,6 @@ exports[`EuiInMemoryTable with items and message - expecting to show the items 1 }, ] } - itemIdToExpandedRowMap={Object {}} items={ Array [ Object { @@ -181,7 +178,6 @@ exports[`EuiInMemoryTable with message 1`] = ` }, ] } - itemIdToExpandedRowMap={Object {}} items={Array []} noItemsMessage="where my items at?" onChange={[Function]} @@ -200,7 +196,6 @@ exports[`EuiInMemoryTable with message and loading 1`] = ` }, ] } - itemIdToExpandedRowMap={Object {}} items={Array []} loading={true} noItemsMessage="Loading items...." @@ -220,7 +215,6 @@ exports[`EuiInMemoryTable with pagination 1`] = ` }, ] } - itemIdToExpandedRowMap={Object {}} items={ Array [ Object { @@ -262,7 +256,6 @@ exports[`EuiInMemoryTable with pagination and default page size 1`] = ` }, ] } - itemIdToExpandedRowMap={Object {}} items={ Array [ Object { @@ -309,7 +302,6 @@ exports[`EuiInMemoryTable with pagination and selection 1`] = ` ] } itemId="id" - itemIdToExpandedRowMap={Object {}} items={ Array [ Object { @@ -361,7 +353,6 @@ exports[`EuiInMemoryTable with pagination, default page size and error 1`] = ` ] } error="ouch!" - itemIdToExpandedRowMap={Object {}} items={ Array [ Object { @@ -401,7 +392,6 @@ exports[`EuiInMemoryTable with pagination, selection and sorting 1`] = ` ] } itemId="id" - itemIdToExpandedRowMap={Object {}} items={ Array [ Object { @@ -477,7 +467,6 @@ exports[`EuiInMemoryTable with pagination, selection, sorting and simple search ] } itemId="id" - itemIdToExpandedRowMap={Object {}} items={ Array [ Object { @@ -547,7 +536,6 @@ exports[`EuiInMemoryTable with pagination, selection, sorting and a single recor ] } itemId="id" - itemIdToExpandedRowMap={Object {}} items={ Array [ Object { @@ -606,7 +594,6 @@ exports[`EuiInMemoryTable with pagination, selection, sorting and column rendere ] } itemId="id" - itemIdToExpandedRowMap={Object {}} items={ Array [ Object { @@ -698,7 +685,6 @@ exports[`EuiInMemoryTable with pagination, selection, sorting and configured sea ] } itemId="id" - itemIdToExpandedRowMap={Object {}} items={ Array [ Object { @@ -748,7 +734,6 @@ exports[`EuiInMemoryTable with sorting 1`] = ` }, ] } - itemIdToExpandedRowMap={Object {}} items={ Array [ Object { diff --git a/src/components/basic_table/basic_table.js b/src/components/basic_table/basic_table.js index 356a30eba42..842b2d764cc 100644 --- a/src/components/basic_table/basic_table.js +++ b/src/components/basic_table/basic_table.js @@ -28,6 +28,7 @@ import { EuiIcon } from '../icon/icon'; import { LoadingTableBody } from './loading_table_body'; import { EuiTableHeaderMobile } from '../table/mobile/table_header_mobile'; import { EuiTableSortMobile } from '../table/mobile/table_sort_mobile'; +import { withRequiredProp } from '../../utils/prop_types/with_required_prop'; const dataTypesProfiles = { auto: { @@ -132,13 +133,14 @@ const BasicTablePropTypes = { columns: PropTypes.arrayOf(ColumnType).isRequired, pagination: PaginationType, sorting: SortingType, - selection: SelectionType, + selection: withRequiredProp(SelectionType, 'itemId', 'see https://github.com/elastic/eui/pull/830'), onChange: PropTypes.func, error: PropTypes.string, loading: PropTypes.bool, noItemsMessage: PropTypes.node, className: PropTypes.string, compressed: PropTypes.bool, + itemIdToExpandedRowMap: withRequiredProp(PropTypes.object, 'itemId', 'see https://github.com/elastic/eui/pull/830') }; export class EuiBasicTable extends Component { @@ -147,7 +149,6 @@ export class EuiBasicTable extends Component { static defaultProps = { responsive: true, noItemsMessage: 'No items found', - itemIdToExpandedRowMap: {}, }; constructor(props) { @@ -476,7 +477,7 @@ export class EuiBasicTable extends Component { } renderItemRow(item, rowIndex) { - const { columns, selection, isSelectable, hasActions, itemIdToExpandedRowMap, isExpandable } = this.props; + const { columns, selection, isSelectable, hasActions, itemIdToExpandedRowMap = {}, isExpandable } = this.props; const cells = []; diff --git a/src/components/basic_table/basic_table.test.js b/src/components/basic_table/basic_table.test.js index 0aeac14bb8d..7aa0d8bd104 100644 --- a/src/components/basic_table/basic_table.test.js +++ b/src/components/basic_table/basic_table.test.js @@ -99,6 +99,7 @@ describe('EuiBasicTable', () => { { id: '2', name: 'name2' }, { id: '3', name: 'name3' } ], + itemId: 'id', columns: [ { field: 'name', diff --git a/src/components/basic_table/in_memory_table.test.js b/src/components/basic_table/in_memory_table.test.js index 519ff8f34c8..a74b973e8a7 100644 --- a/src/components/basic_table/in_memory_table.test.js +++ b/src/components/basic_table/in_memory_table.test.js @@ -102,6 +102,7 @@ describe('EuiInMemoryTable', () => { { id: '2', name: 'name2' }, { id: '3', name: 'name3' } ], + itemId: 'id', columns: [ { field: 'name', diff --git a/src/utils/prop_types/index.js b/src/utils/prop_types/index.js index 43a2fd6873a..85275d1e846 100644 --- a/src/utils/prop_types/index.js +++ b/src/utils/prop_types/index.js @@ -1,7 +1,7 @@ import { is } from './is'; -import { requiresAriaLabel } from './requires_aria_label'; +import { withRequiredProp } from './with_required_prop'; export const EuiPropTypes = { is, - requiresAriaLabel, + withRequiredProp, }; diff --git a/src/utils/prop_types/requires_aria_label.js b/src/utils/prop_types/requires_aria_label.js deleted file mode 100644 index 835cd91518e..00000000000 --- a/src/utils/prop_types/requires_aria_label.js +++ /dev/null @@ -1,27 +0,0 @@ -/** - * PropType validation to require an aria label if the associated function property is also present - * - * example: - * ExampleComponent.propTypes = { - * onClick: PropTypes.func, - * onClickAriaLabel: requiresAriaLabel('onClick') - * } - * - * this validator warns if ExampleComponent is passed an `onClick` prop with no `onClickAriaLabel` - */ -export const requiresAriaLabel = (action) => { - - const validator = (props, propName, componentName) => { - // if the associated action property exists but the aria label property is missing - if (props[action] && !props[propName]) { - return new Error( - `Please provide an aria label to compliment your ${action} ` + - `prop in ${componentName}` - ); - } - - return null; - }; - - return validator; -}; diff --git a/src/utils/prop_types/with_required_prop.js b/src/utils/prop_types/with_required_prop.js new file mode 100644 index 00000000000..f1dba878815 --- /dev/null +++ b/src/utils/prop_types/with_required_prop.js @@ -0,0 +1,35 @@ +/** + * PropType validation that, if the property is present, + * validates against a proptype and verifies that another property exists + * + * example: + * ExampleComponent.propTypes = { + * items: PropTypes.array, + * itemId: withRequiredProp(PropTypes.string, 'items', 'itemId is required to extract the ID from an item') + * } + * + * this validator warns if ExampleComponent is passed an `items` prop but not `itemId` + */ +export const withRequiredProp = (proptype, requiredPropName, messageDescription) => { + const validator = (...args) => { + const [props, propName] = args; + + // run the proptype for this property + let result = proptype(...args); + + // if the property type checking passed then check for the required prop + if (result == null) { + // if this property was passed, check that the required property also exists + if (props[propName] != null && props[requiredPropName] == null) { + result = new Error( + `Property "${propName}" was passed without corresponding property "${requiredPropName}"` + + (messageDescription ? `; ${messageDescription}` : '') + ); + } + } + + return result; + }; + + return validator; +}; From 9acb46f99c7ef4fdeb234eaaa525516f32afe483 Mon Sep 17 00:00:00 2001 From: Chandler Prall Date: Tue, 15 May 2018 11:20:23 -0600 Subject: [PATCH 4/7] added unit test for withRequiredProp --- .../prop_types/with_required_prop.test.js | 79 +++++++++++++++++++ 1 file changed, 79 insertions(+) create mode 100644 src/utils/prop_types/with_required_prop.test.js diff --git a/src/utils/prop_types/with_required_prop.test.js b/src/utils/prop_types/with_required_prop.test.js new file mode 100644 index 00000000000..71bc9b1b469 --- /dev/null +++ b/src/utils/prop_types/with_required_prop.test.js @@ -0,0 +1,79 @@ +import { withRequiredProp } from './with_required_prop'; +import PropTypes from 'prop-types'; + +describe('withRequiredProp', () => { + it('warns when the underlying prop validator fails', () => { + expect(() => { + PropTypes.checkPropTypes( + { + exampleProp: withRequiredProp(PropTypes.string, 'requiredProp') + }, + { + exampleProp: 15 + }, + 'exampleProp', + 'ExampleComponent' + ); + }).toThrowErrorMatchingSnapshot(); + }); + + it('warns when the base prop is present and valid but the required prop is missing', () => { + expect(() => { + PropTypes.checkPropTypes( + { + exampleProp: withRequiredProp(PropTypes.string, 'requiredProp') + }, + { + exampleProp: 'hello' + }, + 'exampleProp', + 'ExampleComponent' + ); + }).toThrowErrorMatchingSnapshot(); + }); + + it('warns with a custom message when validation fails', () => { + expect(() => { + PropTypes.checkPropTypes( + { + exampleProp: withRequiredProp(PropTypes.string, 'requiredProp', 'a custom message') + }, + { + exampleProp: 'hello' + }, + 'exampleProp', + 'ExampleComponent' + ); + }).toThrowErrorMatchingSnapshot(); + }); + + it('does not warn when the base property is missing', () => { + expect(() => { + PropTypes.checkPropTypes( + { + exampleProp: withRequiredProp(PropTypes.string, 'requiredProp') + }, + {}, + 'exampleProp', + 'ExampleComponent' + ); + }).not.toThrow(); + }); + + it('does not warn when both the base property and required properties exist', () => { + expect(() => { + PropTypes.checkPropTypes( + { + exampleProp: withRequiredProp(PropTypes.string, 'requiredProp'), + requiredProp: PropTypes.number + }, + { + exampleProp: 'hello', + requiredProp: 5 + }, + 'exampleProp', + 'ExampleComponent' + ); + }).not.toThrow(); + }); +}); From 93efbd79c63771bc441d5c0921a88f97615de9b6 Mon Sep 17 00:00:00 2001 From: Chandler Prall Date: Tue, 15 May 2018 11:44:24 -0600 Subject: [PATCH 5/7] re-add deleted proptype, commit with_required_prop's snapshots --- src/components/badge/badge.js | 5 +++++ .../__snapshots__/with_required_prop.test.js.snap | 7 +++++++ 2 files changed, 12 insertions(+) create mode 100644 src/utils/prop_types/__snapshots__/with_required_prop.test.js.snap diff --git a/src/components/badge/badge.js b/src/components/badge/badge.js index 8d527c80219..67d10c30e4f 100644 --- a/src/components/badge/badge.js +++ b/src/components/badge/badge.js @@ -175,6 +175,11 @@ EuiBadge.propTypes = { 'Please provide an aria label to compliment your onClick' ), + /** + * Aria label applied to the onClick button + */ + onClickAriaLabel: PropTypes.string, + /** * Accepts either our palette colors (primary, secondary ..etc) or a hex value `#FFFFFF`, `#000`. */ diff --git a/src/utils/prop_types/__snapshots__/with_required_prop.test.js.snap b/src/utils/prop_types/__snapshots__/with_required_prop.test.js.snap new file mode 100644 index 00000000000..8edc8511cd2 --- /dev/null +++ b/src/utils/prop_types/__snapshots__/with_required_prop.test.js.snap @@ -0,0 +1,7 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +exports[`withRequiredProp warns when the base prop is present and valid but the required prop is missing 1`] = `"Warning: Failed exampleProp type: Property \\"exampleProp\\" was passed without corresponding property \\"requiredProp\\""`; + +exports[`withRequiredProp warns when the underlying prop validator fails 1`] = `"Warning: Failed exampleProp type: Invalid exampleProp \`exampleProp\` of type \`number\` supplied to \`ExampleComponent\`, expected \`string\`."`; + +exports[`withRequiredProp warns with a custom message when validation fails 1`] = `"Warning: Failed exampleProp type: Property \\"exampleProp\\" was passed without corresponding property \\"requiredProp\\"; a custom message"`; From b56c9a074a6912789e9c913821861ceef15e3efe Mon Sep 17 00:00:00 2001 From: Chandler Prall Date: Tue, 15 May 2018 11:48:27 -0600 Subject: [PATCH 6/7] update changelog --- CHANGELOG.md | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e3af4821c47..5156fd31ab9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,9 @@ ## [`master`](https://github.com/elastic/eui/tree/master) -No public interface changes since `0.0.46`. +**Breaking changes** + +- Move `EuiBasicTable`'s `itemId` prop from `selection` to a top-level property ([#830](https://github.com/elastic/eui/pull/830)) +- Renamed/refactored `requiresAriaLabel` prop validator to a more general `withRequiredProp` ([#830](https://github.com/elastic/eui/pull/830)) ## [`0.0.47`](https://github.com/elastic/eui/tree/v0.0.47) @@ -15,16 +18,12 @@ No public interface changes since `0.0.46`. - Fixes height calculation error on `EuiAccordion` when it starts loads in an open state. ([#816](https://github.com/elastic/eui/pull/816)) - Added aria-invalid labeling on `EuiFormRow` ([#777](https://github.com/elastic/eui/pull/799)) - Added aria-live labeling for `EuiToasts` ([#777](https://github.com/elastic/eui/pull/777)) -- Added aria labeling requirements for `EuiBadge` , as well as a generic prop_type function `withRequiredProp` in `utils` to check for it. ([#777](https://github.com/elastic/eui/pull/777)) ([#802](https://github.com/elastic/eui/pull/802)) +- Added aria labeling requirements for `EuiBadge` , as well as a generic prop_type function `requiresAriaLabel` in `utils` to check for it. ([#777](https://github.com/elastic/eui/pull/777)) ([#802](https://github.com/elastic/eui/pull/802)) - Ensure switches’ inputs are still hidden when `[disabled]` ([#778](https://github.com/elastic/eui/pull/778)) - Made boolean matching in `EuiSearchBar` more exact so it doesn't match words starting with booleans, like "truest" or "offer" ([#776](https://github.com/elastic/eui/pull/776)) - `EuiComboBox` do not setState or call refs once component is unmounted ([807](https://github.com/elastic/eui/pull/807) and [#813](https://github.com/elastic/eui/pull/813)) - Added better accessibility labeling to `EuiPagination`, `EuiSideNav`, `EuiPopover`, `EuiBottomBar` and `EuiBasicTable`. ([#821](https://github.com/elastic/eui/pull/821)) -- Added `isDisabled` to `EuiComboBox` ([#829](https://github.com/elastic/eui/pull/829)) - -**Breaking changes** - -- Move `EuiBasicTable`'s `itemId` prop from `selection` to a top-level property ([#830](https://github.com/elastic/eui/pull/830)) +- Added `isDisabled` to `EuiComboBox` ([#829](https://github.com/elastic/eui/pull/829)) ## [`0.0.46`](https://github.com/elastic/eui/tree/v0.0.46) From 47340564b20b378f5aac8f2d03d20d2327e15cfe Mon Sep 17 00:00:00 2001 From: Chandler Prall Date: Tue, 15 May 2018 12:54:39 -0600 Subject: [PATCH 7/7] tweaks in response to PR feedback --- src/components/badge/badge.js | 4 ++-- src/components/basic_table/basic_table.js | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/components/badge/badge.js b/src/components/badge/badge.js index 67d10c30e4f..6e497602136 100644 --- a/src/components/badge/badge.js +++ b/src/components/badge/badge.js @@ -158,7 +158,7 @@ EuiBadge.propTypes = { iconOnClick: EuiPropTypes.withRequiredProp( PropTypes.func, 'iconOnClickAriaLabel', - 'Please provide an aria label to compliment your iconOnClick' + 'Please provide an aria label to complement your iconOnClick' ), /** @@ -172,7 +172,7 @@ EuiBadge.propTypes = { onClick: EuiPropTypes.withRequiredProp( PropTypes.func, 'onClickAriaLabel', - 'Please provide an aria label to compliment your onClick' + 'Please provide an aria label to complement your onClick' ), /** diff --git a/src/components/basic_table/basic_table.js b/src/components/basic_table/basic_table.js index f8d6c2cb92c..95fa58924f7 100644 --- a/src/components/basic_table/basic_table.js +++ b/src/components/basic_table/basic_table.js @@ -133,14 +133,14 @@ const BasicTablePropTypes = { columns: PropTypes.arrayOf(ColumnType).isRequired, pagination: PaginationType, sorting: SortingType, - selection: withRequiredProp(SelectionType, 'itemId', 'see https://github.com/elastic/eui/pull/830'), + selection: withRequiredProp(SelectionType, 'itemId', 'row selection uses the itemId prop to identify each row'), onChange: PropTypes.func, error: PropTypes.string, loading: PropTypes.bool, noItemsMessage: PropTypes.node, className: PropTypes.string, compressed: PropTypes.bool, - itemIdToExpandedRowMap: withRequiredProp(PropTypes.object, 'itemId', 'see https://github.com/elastic/eui/pull/830') + itemIdToExpandedRowMap: withRequiredProp(PropTypes.object, 'itemId', 'row expansion uses the itemId prop to identify each row') }; export class EuiBasicTable extends Component {