Move EuiBasicTable's itemId prop out of selection into a top-level prop #830
Conversation
CHANGELOG.md
Outdated
| @@ -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) | |||
There was a problem hiding this comment.
Nit: the link should be wrapped in parentheses to match the others
|
Changes look good but I don't think this fixes the linked issue. While on this branch and messing around with the
Then sorting by name displays the following - which is not what I had expected since another row is expanded.
|
|
@nreese Hmm something weird is going on. I can't repro your UX bug on this branch. In fact, I can't repro it on master either. Not sure how this is happening for you but not for me. |
|
Ah, I didn't update the docs example to fix. I'll also add a proptype to check/warn for this errant combination of props. |
|
Actually, I cannot reproduce what you're seeing either @nreese Still adding that proptype validation. |
|
@cjcenizal @nreese added @snide this changes the aria label prop validator to a more generic validator, but keeps the messaging |
src/components/badge/badge.js
Outdated
| /** | ||
| * Aria label applied to the onClick button | ||
| */ | ||
| onClickAriaLabel: EuiPropTypes.requiresAriaLabel('onClick'), |
There was a problem hiding this comment.
Why did onClickAriaLabel prop get deleted? Looks like it is still being used.
There was a problem hiding this comment.
a bad Cmd+X operation
src/components/badge/badge.js
Outdated
| iconOnClick: EuiPropTypes.withRequiredProp( | ||
| PropTypes.func, | ||
| 'iconOnClickAriaLabel', | ||
| 'Please provide an aria label to compliment your iconOnClick' |
There was a problem hiding this comment.
Typo: "compliment" -> "complement", here and below.
There was a problem hiding this comment.
Can we also change "aria label" to "aria-label" to be more specific about the prop's name?
There was a problem hiding this comment.
The prop it's expecting is iconOnClickAriaLabel, not specifically aria-label. The full warning in the console is
Warning: Failed prop type: Property "iconOnClick" was passed without corresponding property "iconOnClickAriaLabel"; Please provide an aria label to complement your iconOnClick
in EuiBadge
in Unknown
There was a problem hiding this comment.
Gotcha! That makes sense, thanks.
| columns: PropTypes.arrayOf(ColumnType).isRequired, | ||
| pagination: PaginationType, | ||
| sorting: SortingType, | ||
| selection: SelectionType, | ||
| selection: withRequiredProp(SelectionType, 'itemId', 'see https://github.com/elastic/eui/pull/830'), |
There was a problem hiding this comment.
Maybe instead of directing devs to the PR, it would be friendlier to have a message, e.g. "Selection of rows 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') |
There was a problem hiding this comment.
Same idea here, e.g. "Expansion of rows uses the itemId prop to identify each row"
Fixes #820 by refactoring
EuiBasicTable'sselection.itemIdprop into a top-level prop on the table itself. This decouples IDs from selection itself, as the IDs are also used for expanding rows. This is a breaking change that will need some coordination.