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

extra props on EuiInMemoryTable and EuiBasicTable #836

Merged

Conversation

chandlerprall
Copy link
Contributor

@chandlerprall chandlerprall commented May 16, 2018

Fixes #760 and fixes #831 by passing through unknown props on EuiInMemoryTable and EuiBasicTable.

original discussion point / alternative idea The change to `EuiBasicTable`'s render:
const {
  className,
  loading,
  items, // eslint-disable-line no-unused-vars
  itemId, // eslint-disable-line no-unused-vars
  columns, // eslint-disable-line no-unused-vars
  pagination, // eslint-disable-line no-unused-vars
  sorting, // eslint-disable-line no-unused-vars
  selection, // eslint-disable-line no-unused-vars
  onChange, // eslint-disable-line no-unused-vars
  error, // eslint-disable-line no-unused-vars
  noItemsMessage, // eslint-disable-line no-unused-vars
  compressed, // eslint-disable-line no-unused-vars
  itemIdToExpandedRowMap, // eslint-disable-line no-unused-vars
  responsive, // eslint-disable-line no-unused-vars
  isSelectable, // eslint-disable-line no-unused-vars
  isExpandable, // eslint-disable-line no-unused-vars
  hasActions, // eslint-disable-line no-unused-vars
  ...rest
} = this.props;

feels wrong, but with this setup has to happen because those properties are related to logic/functionality internal to basic table.

How do people feel about having a specific prop for the values that are expected to be passed on to children, when the component itself has a large degree of functionality, which could also support multiple targets? E.g. for table you might get <BasicTable items={} tableProps={} rowProps={} cellProps={}> where (table|row|cell)Props are applied to separately to the table, tr, and td elements.

A major downside I see to this is how to communicate to the components' consumers when the separate, explicit property is expected, especially because there's existing complex EUI components that don't follow this pattern (any complex components after May 2018 have extra, otherwise put them directly on the component 🤮).

@cjcenizal
Copy link
Contributor

E.g. for table you might get where (table|row|cell)Props are applied to separately to the table, tr, and td elements.

I'd be really interested to see fleshed-out examples of what kind of props we'd support for which components, and how consumers could use them to do interesting & useful things. I know this example was just to illustrate the idea, but to use it as a strawman I'd like to learn about the use-cases in which adding the same aria-whatever or data-test-subj to each row would be useful. I think adding the same className to each row or cell would be useful, but this can also be accomplished with slightly more specific selectors, e.g. .customTableClass tr (which comes with its own downsides like brittleness and specificity).

Also I want to note that for cases where multiple elements are generated from an array of items (e.g. table rows), we can also treat the item objects themselves as a collection of props. We do this in a few places, e.g. context menu items and horizontal steps. This is really flexible, but it could also be arduous for the consumer to apply a className prop to each row object.

@chandlerprall
Copy link
Contributor Author

Wrote & rewrote a strawman. EuiBasicTable is really the only place this happens, and this seems to indicate it could have been engineered better and not an issue with the ...rest prop strategy itself. Killing that discussion, leaving this PR as a real thing following the existing pattern.

@chandlerprall chandlerprall changed the title WIP: extra props on EuiInMemoryTable and EuiBasicTable extra props on EuiInMemoryTable and EuiBasicTable May 17, 2018
Copy link
Contributor

@jen-huang jen-huang left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants