EuiTableOfRecords component #250

uboness · 2017-12-22T20:45:30Z

EuiTableOfRecords is a high level component that aims to easy the way one creates a table of... (wait for it....) records!!!

What is a record? Think of a record in a data store - typically it represents an entity with a very clear schema. It is something that can be presented in a tabular form.

The goal of this high level component is to make the consumer not think about design. Instead, the consumer only need to define the functional requirements of the table (features), the data, and the type of interaction the user should have with the shown records.

This high level component is as stateless as it needs to be. Meaning, all the management of the data (where it's coming from), record selection and loading pages... all of these are expected to be managed externally to this component. Typically one would use a container component to wrap around this table that will either manage this state internally to that component, or use other state stores such as Redux.

This initial commit includes:

A first implementation of the EuiTableOfRecords
Support for pagination
Support for record selection
Support for custom rendering of the record data (introducing "value renderers")
Supports adding actions per record

This commit also comes with a dedicated documentation section in the EUI doc site.

cjcenizal

🚀 🌕 This is so, so cool @uboness. I'm excited by this initial foray into such a complicated component. I focused my first round of feedback on the ergonomics of the component by looking at just a single example and feeling the code out. What kind of mental model do I form by reading this code? Can I make reasonable assumptions about how to use feature B based on the use of feature A? Are we introducing new concepts and if so, what do we gain from them? These are the kinds of questions I asked myself, and the suggestions I made are geared towards smoothing some of the rough edges I found.

I also think we should try to slim down the documentation. It's very thorough but too much documentation risks overwhelming the reader and thus not being read. I made some comments about ways we can do this.

I didn't even get into the implementation details of EuiTableOfRecords. I think once we're happy with the ergonomics of these interfaces I'll be able to take a close look at the implementation and provide some sound feedback.

cjcenizal · 2017-12-29T01:00:30Z

src-docs/src/views/table_of_records/multiple_record_actions.js

+  deletePerson(personToDelete) {
+    const i = people.findIndex((person) => person.id === personToDelete.id);
+    if (i >= 0) {
+      people.splice(i, 1);


This side effect throws me off a bit. Are you using this method to simulate logic you'd expect to be carried out server-side? If so then I think we just need to clarify the boundary between the component and "calls to the server" so that this example is easier to understand. I'd use a combination of comments to explain what's happening on the server vs. client, and setTimeout to simulate the request/response delay.

If this wasn't the intention of this method (and similar methods like clonePerson), then I'd say it's unlikely we'd perform a synchronous update of our data within a callback like this. And if we did, I think we'd want to change the flow of logic so that the callback calls setState to set the new people state (and any other values required for our minimal representation of UI state), and then we'd derive as much data as we could in the render method.

I think we need to discuss the nature of this component as I gather from the last few questions (including this one) that it's totally not clear. I tried to explain it in the "What is a high level component" call out section.. perhaps I did not do a good enough job.

In a nutshell.. this high level component doesn't (nor should) care about where the data is managed, whether we hold it in memory or wether we load it remotely... or wether there's a delay in loading it. It is the least important part of the examples here. The important part is the props you pass to the component and the callback you register. for the rest, there can be many ways to deal with data... but this is really not the point here.

In this specific example, we want to show how to register an action... this action happens to be a "delete person" action... I could have shown an action that just shows an alert dialog with a "hi there ${person.name}" message.... but I didn't... I wanted to make it more realistic so I implemented a "deletePerson" action... now because in the example we hold all people in an array in memory, the implementation is a simple splice... but really... who cares... it could have been something completely different...

cjcenizal · 2017-12-29T20:29:28Z

src-docs/src/views/table_of_records/multiple_record_actions.js

+  }
+
+  computeState(criteria) {
+    const page = loadPage(criteria.page.index, criteria.page.size, criteria.sort);


Can we pass pageIndex, pageSize, sortKey, and sortDirection around as arguments instead of the criteria object? This would make the logic more transparent for me because I wouldn't need to know what the anatomy of the criteria object is or the page or sort child objects.

sure, we can... I don't mind... but again, this is the least important part of the example one should focus on

cjcenizal · 2017-12-29T20:34:11Z

src-docs/src/views/table_of_records/multiple_record_actions.js

+        },
+        sort: criteria.sort
+      }
+    };


For simplicity's sake, I think we should just move the loadPage logic into this method. I don't see a reason to extract it.

oh... but do see a reason :)... it has to do with the fact that with this example, we're using the component's state to store the necessary state for the table... loading the data however is a completely separated concerns. I think it's important to separate these two concerns and not promote tying them to one another.

cjcenizal · 2017-12-29T23:37:55Z

src-docs/src/views/table_of_records/multiple_record_actions.js

+
+    };
+
+    return <EuiTableOfRecords config={config} model={this.state}/>;


One takeaway I get from this code is that it's really useful to be able to "bundle" certain props together into an object which adds or removes functionality, e.g. sorting, selectability, and pagination.

On the other hand I think we should avoid overly regimenting our props. My thinking is the flatter our props, the less of a DSL people need to learn in order to use this component. I was thinking of something like this:

const columns = [{ key: 'firstName', name: 'First Name', description: `Person's given name`, dataType: 'string', sortable: true }, { key: 'lastName', name: 'Last Name', description: `Person's family name`, dataType: 'string' }, { /* ... */ }]; const pagination = { onPageChange: /* put a callback here */, pageSizeOptions: [3, 5, 8] }; const selection = { onSelectionChange: /* put a callback here */, selectableMessage: person => !person.online ? `${person.firstName} is offline` : undefined }; const sort = { onSortChange: /* put a callback here */, }; // I image that in a subsequent step we’ll want to support isFetching, isEmpty, and isError states return ( <EuiTableOfRecords recordId="id" isFetching={this.state.isFetching} totalRecordCount={this.state.totalRecordCount} columns={columns} rows={this.state.recordsOnPage} pagination={pagination} selection={selection} sort={sort} /> );

separating the callback doesn't work well... I had it separated initially and moved away from that. The main reason is that pagination, sorting and (later) filtering are all facets of the same concerns, which is basically defining the criteria of the data you're viewing. How does the table behave when you sort? do you clear selection - do you reset pagination? how does the table behave when you pagination - do you clear selection do you keep the sorting? these are all questions you want to avoid consumer asking or even dealing with. This behaviour needs to be determined and dictated by the component itself - this is the only way we can create consistent behaviour across the board - it's not just about documenting our design and UX patterns, it's also about trying to enforce them whenever we can... and if we can't enforce them, then at least promote them. Moving away from these patterns should be an exception that requires addition work and should be well documented why its done in the first place. To achieve this, you need to have a single callback that notifies about a change in the "criteria"... consumers don't know what drove this change, nor they should care about it... all they know is that the table is "requesting" to update the data based on a new criteria (it could be that the user sorted and because of that the table also decided to reset pagination... or that the user added a filter which caused a pagination reset as well... but you don't know... and it's ok... these are the rules and behaviours the table defines and takes care of for you).

personally I don't think isFetching should be the concern of the table... it doesn't really know what "fetching is"... if you use an in-memory model around it, it's meaningless... if you use async/remote model around it it has a meaning... is fetching is a higher level concern... higher than this table.

The flattening of this model breaks it's high level and brings it lower than intended. At a high level you deal with "data" which is: 1. a list of records, and 2. the criteria that describes these records. This is what the table cares about - data... not rows. Again... I feel we need to have a discussion about the goal of the component in the first place and how the users/consumers should look at it.... I feel there's a fundamental miss in the overall intention here, that (I guess) I miss out when trying to explain it.

cjcenizal · 2018-01-05T19:48:17Z

src-docs/src/views/table_of_records/multiple_record_actions.js

+
+  constructor(props) {
+    super(props);
+    this.state = this.computeState({


What kind of data flow are we trying to simulate in this example? Are we mimicking data fetched from a server API? Or working with a local data store? I'll be able to provide better feedback once I understand the intent.

it matters not for this component, so we're not trying to mimic anything. As far as the consumer is concerned this component is stateless, meaning you need to provide all the necessary info for it to work as props. For this reason, the data can be loaded from the server async, or it can be stored in memory... but for this component it really doesn't matter therefore focusing on this question makes little sense to me

cjcenizal · 2018-01-05T21:56:43Z

src/components/value_renderer/date/date.js

+};
+
+export const date = createDateRenderer();
+date.with = (config) => createDateRenderer(config);


It seems like there are two types of renderers: those which return a component, and those which simply humanize values. I think it makes sense to clearly divide the two, and publish the former as true React components in the components directory, and the latter as functions in the services directory. I think this will make it easier for the consumer to understand how each works and compose them at will.

This particular renderer seems better suited as a service.

Yeah render to me in a React app means returning a React component. A lot of these renderers are what I would call formatters.

cjcenizal · 2018-01-05T21:59:01Z

src/components/value_renderer/default_renderer/defaultRenderer.js

+import { text } from '../text';
+import { booleanText } from '../boolean';
+
+export const defaultRenderer = (value) => {


I'm not sold on the value of having so much functionality packed into a single function. I can see its use as a diagnostic or development tool when you're prototyping out a feature or working with mock data. But when would we use this in production?

This is used internally by the components as the default renderer when no rendering hints or customisation is provided by the consumer. without it, every time you'd want to have defaults you'll find yourself if-elsing repetitively - this a one stop shop for default value rendering.

cjcenizal · 2018-01-05T22:02:37Z

src/components/value_renderer/health/health.js

+    const color = config.color(value, ctx);
+    return <EuiHealth color={color}>{resolvedContent}</EuiHealth>;
+  };
+};


What's the advantage of using this renderer over the EuiHealth component directly?

render: (value) => { const color = value === 0 ? 'danger' : 'success'; return ( <EuiHealth color={color}>{value}</EuiHealth> ); },

not a huge advantage... but in the same line as in link() or using descriptors for buttons... having this as the default goto will promote consistency (e.g. consumers won't immediate start thinking about providing their own components for representing health status)... maybe a better name would be "status" rather than health... as it's more the idea we want to convey rather than talk about the actual implementation... yea... the more I think about it (write these lines) the more I like "status" :)

cjcenizal · 2018-01-05T22:03:49Z

src/components/value_renderer/link/link.js

+    const onClick = () => config.onClick(value, ctx);
+    return <EuiLink type={config.type} color={config.color} onClick={onClick}>{resolvedContent}</EuiLink>;
+  };
+};


Same here, why not just use EuiLink directly?

cjcenizal · 2018-01-05T22:07:09Z

src/services/sort/index.js

@@ -1 +1,4 @@
 export { SortableProperties } from './sortable_properties';
+export { SortDirectionType, SortDirection } from './sort_direction';
+export { PropertySortType } from './property_sort';
+export { Comparators } from './comparators';


These new services seem to duplicate the functionality of SortableProperties. Is there any way we can use the existing service?

tbh I'm really not a big fan of SortableProperties (it ties to a discussion we recently had about the meaning of the extra arrows). We can simplify things quite a bit here... we only sort on one field at a time, so no need to keep state for all the other sorts... the "sortable" indicator needs to be a different icon than what we have today (and then you don't have the need for keeping the extra state).

Look how simpleSortDirection is when you don't need state compared to SortableProperties...

cjcenizal · 2018-01-05T22:30:06Z

I forgot to mention I found a few other issues:

Row action accessibility

I wasn't able to tab to the row actions, which means they're inaccessible to people who use the keyboard to navigate. We can solve this by leaving the actions in the DOM and using CSS to set their opacity to 1 when the row is hovered or the button is focused.

Search and bulk actions

Were you planning on also bundling the search input and bulk actions into this component? I think these are worth adding.

Table states

We'll need to support loading, error, and empty states with our tables. We'll need input from @cchaos and @snide on how these should appear. I think a good, safe choice would be to simply allow the consumer to pass in a React component to a prompt prop, which can be specified at any time for any reason. This would be a good catch-all for all loading, error, and empty states, and any additional ones which arise in the future.

Sorting behavior

Can we standardize our sorting behavior based on elastic/kibana#9865? This is something provided by the SortableProperties service.

uboness · 2018-01-07T00:06:25Z

I also think we should try to slim down the documentation. It's very thorough but too much documentation risks overwhelming the reader and thus not being read. I made some comments about ways we can do this.

Personally I don't agree... I think that the docs need to convey how one can use a component and not assume everything from one example (can I do X... can I do Y... one example assumes the user knows all the options and the required props you must provide or the optional/required feature a component supports). I personally like having a build up where you start with the bare minimum and you enhance things gradually. You still provide ppl with an option to directly jump to the most "featured" example... but you also let them see they can use it in different ways.

This doesn't really tie to this component though, this is something we need to decide on a much higher level for this framework as a whole. I'd love to hear @snide 's opinion on it, but also other kibana engs that eventually use it and are the target audience for this docs.

uboness · 2018-01-07T00:09:25Z

@cjcenizal thanks for taking the time to start the review... exciting times... I'm in the process of answering a lot of your questions... it'll take some time though :)

uboness · 2018-01-07T02:31:41Z

Row action accessibility

Absolutely... Known issue... Discussed with Dave about it... I know how to fix this 🙂

search and bulk actions

Absolutely... In a follow-up pr once this is in (it gotten quite big already)

table states

Absolutely... In a follow-up pr

cjcenizal · 2018-01-08T18:39:22Z

To support the ability to use dataType as well as append a data-test-subj attribute to a cell, maybe we can support a props() callback alongside the render() callback. You can only use one or the other. In the props() callback you can return a props object which will be passed to the td element.

cjcenizal · 2018-01-19T20:41:02Z

I've submitted a PR with some suggestions: https://github.com/uboness/eui/pull/1/

cjcenizal · 2018-01-20T00:26:16Z

@uboness I noticed a bug with sorting... not sure what's causing this, but it looks like the direction of the arrow is the reverse of the sort direction (ascending === A-to-Z === arrow points up):

uboness · 2018-01-20T02:25:05Z

Hmm... It's actually intentional... I mean, I didn't look at common/other implementations... It just made sense to me that asc is down... Even looking at the screenshot above makes total sense to me. But if we need to change that, we can of course

bmcconaghy

This is looking really nice. I added a few comments here and there, but mainly minor stuff.

bmcconaghy · 2018-01-22T17:26:53Z

src/components/table_of_records/table_of_records.js

@@ -0,0 +1,752 @@
+import React from 'react';
+import * as _ from 'lodash';


this works too:
import _ from 'lodash'

bmcconaghy · 2018-01-22T17:39:09Z

src/components/table_of_records/table_of_records.js

+
+  togglePopover(id) {
+    this.setState((prevState) => ({
+      ...prevState,


You don't need this. setState merges the result of this method with the current state, so ...prevState will already be in there.

strange... I was sure that the merge only happens when you provide a value to setState, and that it's your responsibility to merge when providing a callback.. but it looks like I was wrong:

Both prevState and props received by the updater function are guaranteed to be up-to-date. The output of the updater is shallowly merged with prevState.

will change

bmcconaghy · 2018-01-22T17:44:53Z

src/components/table_of_records/table_of_records.js

+      const checked = this.state.selection && this.state.selection.length > 0;
+      const onChange = (event) => {
+        if (event.target.checked) {
+          const selectableRecords = model.data.records.reduce((records, record) => {


I think this is more readable as:

model.data.records.filter((record) => !config.selection.selectable || config.selection.selectable(record));

heh... no sure why I didn't do that in the first place :) 👍

bmcconaghy · 2018-01-22T18:10:21Z

src/components/table_of_records/table_of_records.js

@@ -0,0 +1,752 @@
+import React from 'react';


This file seems pretty large and to have a number of responsibilities. Seems like breaking this out into smaller components would help.

I thought about it a few times... but always came back to the same conclusion. The thing that makes this file large is the rendering logic and it feels awkward to break it apart. Personally I like having all rendering logic in one file... but if ppl insist I can break it apart a bit (not too much though... maybe to table, footer and later header).... but again.. personally I'd vote for keeping all rendering logic in one place

Yeah I was thinking table header and footer as the breakout.

bmcconaghy · 2018-01-22T18:13:14Z

src/components/value_renderer/date/date.js

+};
+
+export const date = createDateRenderer();
+date.with = (config) => createDateRenderer(config);


Yeah render to me in a React app means returning a React component. A lot of these renderers are what I would call formatters.

chrisronline · 2018-01-19T17:46:42Z

package.json

@@ -23,12 +23,14 @@
    "brace": "^0.10.0",
    "classnames": "^2.2.5",
    "core-js": "^2.5.1",
+    "date-fns": "^1.29.0",


Should we just use moment, as Kibana already uses it? Does this do something moment can't?

I thought about that... wasn't sure if we plan to continue with moment or not (I remember some debate around it... if the plan is to continue using it... sure, I'll update, but if the plan is to slowly remove moment.js, then we better not... hmm...

@kjbekkelund / @epixa what's the status of moment.js in Kibana? is it safe to use?

(btw... I'd be super happy to use moment.js... it's a joy next to anything else when dealing with dates)

got the green light to use moment.js... will make the change

@chrisronline done

Sweet, thanks!

chrisronline

I still want to review the implementation but from the examples on the demo docs, this looks amazing! Awesome job!

chrisronline · 2018-01-23T01:27:05Z

src/components/table_of_records/table_of_records.js

+        size
+      }
+    };
+    this.props.config.onDataCriteriaChange(criteria);


I'm going to think about it more, but this feels weird to me. I'm fairly sure I understand the what and why, but it feels awkward to remember to tell someone else that the criteria changed, but when it seems like the only thing that will care is the TableOfRecords itself? Is this just so the consuming component can manage the list? Am I making any sense? :P

:)... I can totally understand how you feel about it, and you're probably missing context here... so here it goes...

The point with this table was to make it as stateless as possible. The main reasons for this is that it should be possible to use it in any environment regardless of where and how one stores state.

Some state, is totally internal to the table (selection is a good example) and with this type of state we indeed keep it within the table itself and don't leak it out (the user can still register a callback method to "listen" to selection changes, but it's optional).

With data, things are a bit different. Data is not typically help in memory and one can manage it it many ways. For example, one can have a higher level component (container component) that stores data (or some of it) in it's own internal state. Alternatively one can use a redux store to store the state. This is where this specific table stops making the decisions for the user. I want to enable ppl to use this table with redux or however they choose to store the state.

That said, we do plan to provide a higher level container on top of this table that will handle most of the state for the user. It could be very useful to have this component, specially for K6 (reduxless env) and also for use cases where one actually wants to hold all data in memory. The plan was to add that container (currently named TableOfRecordsDS) in a later commit, once this goes in.

but when it seems like the only thing that will care is the TableOfRecords itself?

you'll get that with TableOfRecordsDS... well... mostly... my plan is that it'll be flexible enough to either hold everything in memory (in its state), or delegate the data loading to an async function returning a promise... on of the two

Is this just so the consuming component can manage the list?

either the consuming component or perhaps a redux store... or something else... trying not to make it the concern of this table.

Am I making any sense? :P

yes, you do :).... it all started when I came from the world of redux and best practices there (one things we found out that with redux it's best to work with stateless components and let the state be stored in a single place rather than scatter it all over the place). So it was important for me to make sure this works well in that env.... but I totally recognize the need for a component that will be "fully managed" or "mostly managed"... hence TableOfRecordsDS

btw... I need help with finding a good name for TableOfRecordsDS 🤦‍♂️

Gotcha, yea that totally makes sense. I totally agree about adding another HoC to facilitate a couple common use cases of managing the records. Thanks for clarifying!

uboness · 2018-01-23T01:29:31Z

@cjcenizal I'm back on the fence wrt requiring the pagination config to enable pagination. The main reason for this:

There should only be one thing that enables a feature.. the minute you start sending ppl to change multiple things it because a bit of a mess
code-wise we now need to validate proper use by making sure that if the model indicates paging but the table isn't configured with pagination we should throw an error - it's a misuse and/or confusion with the consumer.
pagination needs to be enabled by default... this is not possible now that we require config.pagination to be configured. We could solve that by setting assuming pagination and letting the user to opt-out of it using config.pagination.enabled: false setting... but that pretty much brings us to the beginning - I much rather tell the user "you want to opt out of pagination? don't provide the page criteria" that's it.

model.criteria.page can be the only thing that enables/disabled pagination.... for further configuration of it (that is... configuring how the pagination behaves) we can expose as config.pagination setting (a la page size options)

cjcenizal · 2018-01-23T02:25:46Z

@uboness Hehe this is why I wanted to break up and recombine the parts of configuration and model, so we could group things into cohesive objects, e.g. a single pagination object which contains all relevant props. Then we could just add/remove that to enable/disable pagination.

Why does pagination need to be enabled by default, btw?

uboness · 2018-01-23T02:41:40Z

@uboness Hehe this is why I wanted to break up and recombine the parts of configuration and model, so we could group things into cohesive objects, e.g. a single pagination object which contains all relevant props. Then we could just add/remove that to enable/disable pagination. Aside from that, the benefit of consolidating the criteria facts under one object is consistency - we expose the onModelCriteriaChange event where the consumer get the criteria object... it's somewhat simple to think as this object as an atom where the same criteria object is also provided back to the table. Note, I didn't have onModelCriteriaChange and instead I indeed broke things by feature (e.g the user could listen in to pagination events with onPaginationChange callback within the pagination object... but that was a problematic approach - it doesn't provide us full control over the behaviour of the table (for example, we want to make sure pagination is reset when the user sorts... we can only do that if we have expose both sorting & pagination in a single object with a single callback... the alternative will mean there'll be multiple requests and flickery UI)

also... it'd feel strange that the page criteria will be apart from the data... these two really belongs together IMO... you have the data and what describes the data in one place.

Why does pagination need to be enabled by default, btw?

I stated based on the places I already foresee this component being used you'd want pagination in all of them... so it feels more like an opt-out feature rather then an opt-in one. Even for those use cases that I do not foresee (yet)... as long as we don't support infinite scrolling, or something like that, I think for proper UX you'd almost always want pagination. But regardless of that, if we only rely on the page criteria to enable disable pagination, it's no longer a matter of whether the user opts in/out of it... if the user attaches page criteria we'll enable it... if not, we won't... simple

uboness · 2018-01-23T06:30:34Z

@uboness I noticed a bug with sorting... not sure what's causing this, but it looks like the direction of the arrow is the reverse of the sort direction (ascending === A-to-Z === arrow points up):

@cjcenizal fixed... the issue was in the comparators (added tests).... I wonder if we could use a better icon to indicate sort direction... something like:

chrisronline

At a high level, this looks fantastic and I think the direction is solid. I'm a huge fan of the work done! I'll spend some more time reviewing details but this is great!

cjcenizal

LGTM! I'm OK merging this as-is, but I personally would still like to group the props by concern (similar to this recommendation).

This is a strong opinion weakly held so I'd like to hear your thoughts @bmcconaghy @chrisronline. Do you feel strongly like an interface like the one below would be an improvement? Or do you feel like it's a lateral or even backwards move?

constructor(props) {
  super(props);

  this.state = {
    ...this.computeState({
      pagination: {
        index: 0,
        size: 5,
        pageSizeOptions: [3, 5, 8],
      },
    })
  };

  this.columns = [
    {
      field: 'firstName',
      name: 'First Name',
      description: `Person's given name`,
      dataType: 'string',
      sortable: true,
    },
    /* ... */
  ];

  this.selection = {
    selectable: (record) => record.online,
    selectableMessage: person => !person.online ? `${person.firstName} is offline` : undefined
  };
}

render() {
  const {
    rows,
    pagination,
    sort,
  } = this.state;

  return (
    <EuiTableOfRecords
      recordId="id"
      columns={this.columns}
      rows={rows}
      selection={this.selection}
      pagination={pagination}
      sort={sort}
      onDataCriteriaChange={(criteria) => this.onDataCriteriaChange(criteria)}
    />
  );
}

chrisronline · 2018-01-23T18:48:05Z

@cjcenizal Feels lateral to me. I don't honestly have a strong preference either way.

uboness · 2018-01-26T12:24:29Z

@cjcenizal I've pushed the changes we worked on yesterday. Added tests for all the new components I created and also updated the docs. As we said, you still need to look at the updateFocus() method we disabled in context menu.

@bmcconaghy cj wills till work on fixing the build (we had to comment out the updateFocus method in context menu as it was in the way for properly handling focus/blur events). That said, I split the code as discussed and a bit more than just the footer (which is now named PaginationBar... also split the code for the record actions. overall I think it's a good balance (as mentioned somewhere above, I'm not keen on splitting the rendering of table itself)

kimjoar · 2018-01-28T01:51:52Z

src-docs/src/views/table_of_records/implicit_record_action.js

+          description: `Person's nickname / online handle`,
+          render: EuiValueRenderers.link({
+            onClick: (value) => {
+              window.open(`http://www.github.com/${value}`, '_blank');


This breaks accessibility, I think: https://developer.mozilla.org/en-US/docs/Web/API/Window/open#Never_use_<a_href_onclickwindow.open(...)>

I know this is our docs and not Kibana itself, but we likely shouldn't use bad patterns in our docs either.

will be fixed soon

`EuiTableOfRecords` is a high level component that aims to easy the way one creates a table of... (wait for it....) records!!! What is a record? Think of a record in a data store - typically it represents an entity with a very clear schema. It is something that can be presented in a tabular form. The goal of this high level component is to make the consumer not think about design. Instead, the consumer only need to define the functional requirements of the table (features), the data, and the type of interaction the user should have with the shown records. This high level component is as stateless as it needs to be. Meaning, all the management of the data (where it's coming from), record selection and loading pages... all of these are expected to be managed externally to this component. Typically one would use a container component to wrap around this table that will either manage this state internally to that component, or use other state stores such as Redux. This initial commit includes: - A first implementation of the `EuiTableOfRecords` - Support for pagination - Support for record selection - Support for custom rendering of the record data This commit also comes with a dedicated documentation section in the EUI doc site.

General Services: - introduced a `utils` directory for general utilities (which are not really services) - introduced `EuiPropTypes` - our own extension to `PropTypes` - introduced `EuiPropTypes.is` - a type validator that expect an exact match on a specific value - introduced `ASC` and `DESC` as sort direction values and `SortDirectionType` react type for it - introduced `PropertySortType` - a react type for an object representing meta data for property sort (key and direction) Features: - Selection is now managed internally in the table component itself. Selection can be seen as an internal UI facet of the component rather than a general "date" facet. It is one less thing for the consumer to manage, yet they can still register an `onSelectionChanged` callback method and be aware and react to selection changes (so it's a win-win) - introduced the notion of "computed columns". Until now we only had "data and "actions" column types. Now, with "computed" column, one can define a column that is not associated with any specific key and provide a renderer that renders the record's column content derived from the record itself. Not that computed column are not sortable. Docs Examples: - `Single Record Action` - shows a table where the rows have a single action associated with them. - `Multiple Record Actions` - shows a table where the rows have multiple actions associated with them. - `Implicit Record Action` - shows how one can utilize "computed" columns to add custom controls to none action columns

General Services: - Introduced `SortDirection` - a constant that holds the direction constants + a few helper functions around directions - introduced `Comparators` - a start of a set of comparators to be used for sorting values - introduced `EuiPropTypes.is` - a type validator that expect an exact match on a specific value - introduced `ASC` and `DESC` as sort direction values and `SortDirectionType` react type for it - introduced `PropertySortType` - a react type for an object representing meta data for property sort (key and direction) Features & Fixes: - Added an option to specify a selectable message - can be used to deliver a reason for why a record is not selectable (shown as the title/tooltip of the checkbox) - Fix: Paginating (changing page size or navigating through the pages) and Sorting will now clear selection. Docs Examples: - Added a `Sorting` section + an example of how you set up column sorting - Enhanced the `Selection` example to also provide a message explaining why a selection is disabled (shown as a title on the checkbox)

Previously the config enabled you to define multiple separate callbacks for pagination and sorting. The problem with that approach is that it still left too much for the user to manage. Specifically, certain behaviour that we'd like to promote and control from the `EuiTableOfRecords` component, was left to the user to implement - For example, one of the design patterns decision we had is to reset pagination when sorting changes. Unfortunately this cannot be done from within the component with multiple separate callbacks. For this reason, the config and model props changes quite a bit. Now there's a clear separation between the data itself and the criteria that describes and loaded the data. The new model structure looks like this: ```js { data: { records: [], // list of records totalRecordCount: number }, criteria?: { page?: { index: number, size: number }, sort?: { key: string, direction: 'asc' | 'desc' } } } ``` In addition, there now only a single callback that asks the consumer to update the data - `onModelCriteriaChange`. This enables the table component to decide how the criteria change based on the interaction of the user - in other words, the table now controls its own behaviour. Nice side effects: - Pagination is enabled automatically when the `page` criteria is provided in the model (no need to explicitly configure pagination in the "config") - Sorting is enabled automatically when one of the columns is marked as `sortable` (no need to explicitly configure sorting` in the "config") - Rendering a table that shows all data (without pagination) is a simple as passing in the data - that is `model.data` (no need to pass in additional single page meta data as before). - No more need for the `Page` construct Examples: - added a proper intro to the examples page - explaining the `config` and the `model` concepts. Also providing high level explanation of what exactly is a "High Level Component" (as it's relatively new to EUI). - Updated all the examples based on the new model

- moved `value_renderer` module to live under `components` (to keep all renderers centralized and avoid circular dep.) - removed `.isRequired` for `model.criteria.page` prop - removed `.isRequired` for `name` property of an actions column - added proper error messages if the `onDataCriteriaChange` is not defined yet pagination and/or sorting is enabled - added proper error messages if a column data type is unknown - fixed some code formatting issues

- fixed accessibility for "hidden" record actions. Now using `opacity: 0` instead of `visibility: hidden`. - updated the docs... well... reduced the docs to 2 examples (for easier digestion during PR review process)

* Add ability to toggle on and off different features of the table. * Rename ValueRenderers to EuiValueRenderers. * Add 'Column render types' example. * Hide footer when missing necessary config. Rename key prop to id. * Refer to dataType instead of renderType. * Rename column.id -> column.field.

- better/cleaner support undefined values in date & number value renderers - better error reporting by the table in case it's misconfigured - ensure unique component `key` creation within the table - introduced the `Random` service - cleans up the text (would be great if we later take this and build randomized testing around it)

- cleaned up imports - rely on the shallow merging of `setState` when providing a callback - use `.filter(...)` when appropriate (instead of `.reduce(...)`

- removed dependency on `date-fns` - added `moment.js` as peer & dev dependency - reimplemented the `date` value renderer using momentjs - added support for `calendar`, `calendarDate` and `calendarDateTime` formats

- `model.data.totalRecordCount` is no longer required (defaults back to the length of `model.data.records`) - renamed `renderFooter` to `renderPaginationBar`

- since the default props docs don't support rich props hierarchy, we construct the docgen info ourselves. - changes the styling of the props docs - introduced a special `_euiObjectType` field in docgenInfo "mark" the documented component as a "type" rather than a react component - added internal links between the different types

- also renamed `propsInfo.js` to `props_info.js`

…ring and cleanup: - Now we properly handle focus/blur event delegation among the table, expanded record actions and collapsed actions - collapsed action is now rendered as a single custom action - code split: created dedicated components for `ExpandedRecordActions`, `CollapsedRecordActions`, `DefaultRecordAction`, `CustomRecordAction`, and `PaginationBar`. - Snapshot tests were added to all the new components - Simplified the action configuration - it's no more needed to default an action type ("custom" is inferred by the `render` method), also, the button and icon actions are now one (they're distinguished by the now optional `type` field) - Updated the `props_info.js` docs to reflect all these changes - props docs infrastructure now support code markup for descriptions and comments on default values. - `EuiPopover` now support passing a `popoverRef` prop to get a handle to the popover element. - Cleaned up and simplified the table of records examples

…n EuiContextMenuPanel. (#3)

- context menu is currently experiencing difficulties with focusable elements as items. So util this is fixed (in #336) we should not encourage using those. - while at it, renamed the full featured example js file to `full_featured.js` (used to be something I already managed to forget)

* Delete Link and Health value renderers. Convert Date value renderer into the formatDate service. * Convert defaultRenderer tests to use snapshots so they're testing interface instead of implementation. Convert file name to snake case. * Inline default renderer test values instead of using snapshots. * Delete BooleanIon renderer. Convert BooleanText renderer into formatBoolean service. * Convert Number renderer into formatNumber service. * Convert Text renderer into formatText service. * Convert Default renderer into formatAuto service. Delete EuiValueRenderers module. * Duck-type formatNumber arguments. * Use isString instead of typeof. * Change references from 'default' to 'auto'.

- removed redundant comments - aligned how we name keys across the boar (consistent key value pattern)

uboness requested review from snide and cjcenizal December 28, 2017 04:16

chrisronline mentioned this pull request Dec 29, 2017

[META] EUIfication elastic/kibana#15721

Closed

47 tasks

cjcenizal suggested changes Jan 5, 2018

View reviewed changes

cjcenizal requested a review from bmcconaghy January 11, 2018 20:44

cjcenizal requested review from chrisronline and timroes January 19, 2018 17:16

timroes mentioned this pull request Jan 22, 2018

Implement new inspector elastic/kibana#16185

Closed

6 tasks

cjcenizal requested a review from nreese January 22, 2018 16:34

bmcconaghy reviewed Jan 22, 2018

View reviewed changes

chrisronline reviewed Jan 22, 2018

View reviewed changes

chrisronline reviewed Jan 23, 2018

View reviewed changes

chrisronline approved these changes Jan 23, 2018

View reviewed changes

cjcenizal approved these changes Jan 23, 2018

View reviewed changes

uboness changed the title ~~Initial commit of EuiTableOfRecords component~~ EuiTableOfRecords component Jan 26, 2018

cjcenizal mentioned this pull request Jan 26, 2018

EuiContextMenuItem needs to accept arbitrary components as children #336

Closed

kimjoar reviewed Jan 28, 2018

View reviewed changes

uboness and others added 20 commits January 29, 2018 21:29

[EuiTableOfRecords] cleaned up and added tests for all value renderers

14c180b

applied review feedback

ca04108

- fixed accessibility for "hidden" record actions. Now using `opacity: 0` instead of `visibility: hidden`. - updated the docs... well... reduced the docs to 2 examples (for easier digestion during PR review process)

fixed broken build after rebase

868ec1a

Applied feedback:

b1bd169

- cleaned up imports - rely on the shallow merging of `setState` when providing a callback - use `.filter(...)` when appropriate (instead of `.reduce(...)`

Moved to momentjs

a913df2

- removed dependency on `date-fns` - added `moment.js` as peer & dev dependency - reimplemented the `date` value renderer using momentjs - added support for `calendar`, `calendarDate` and `calendarDateTime` formats

A bit of a cleanup:

d50ef6b

- `model.data.totalRecordCount` is no longer required (defaults back to the length of `model.data.records`) - renamed `renderFooter` to `renderPaginationBar`

all record callback methods now accept a model along with the record

29473a4

- also renamed `propsInfo.js` to `props_info.js`

Use shouldComponentUpdate to avoid unnecessary calls to updateFocus i…

5ce1e62

…n EuiContextMenuPanel. (#3)

cleanup

994aa64

- removed redundant comments - aligned how we name keys across the boar (consistent key value pattern)

uboness merged commit 099bf48 into elastic:master Jan 30, 2018


		};

		return <EuiTableOfRecords config={config} model={this.state}/>;

		@@ -0,0 +1,752 @@
		import React from 'react';
		import * as _ from 'lodash';

EuiTableOfRecords component #250

EuiTableOfRecords component #250

Conversation

uboness commented Dec 22, 2017 • edited Loading

cjcenizal left a comment

Choose a reason for hiding this comment

cjcenizal Dec 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjcenizal Dec 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjcenizal Jan 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjcenizal Jan 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjcenizal commented Jan 5, 2018 • edited Loading

Row action accessibility

Search and bulk actions

Table states

Sorting behavior

uboness commented Jan 7, 2018

uboness commented Jan 7, 2018

uboness commented Jan 7, 2018

cjcenizal commented Jan 8, 2018

cjcenizal commented Jan 19, 2018

cjcenizal commented Jan 20, 2018

uboness commented Jan 20, 2018

bmcconaghy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uboness Jan 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisronline left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uboness commented Jan 23, 2018

cjcenizal commented Jan 23, 2018

uboness commented Jan 23, 2018

uboness commented Jan 23, 2018 • edited Loading

chrisronline left a comment

Choose a reason for hiding this comment

cjcenizal left a comment • edited Loading

Choose a reason for hiding this comment

chrisronline commented Jan 23, 2018

uboness commented Jan 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uboness commented Dec 22, 2017 •

edited

Loading

cjcenizal Dec 29, 2017 •

edited

Loading

cjcenizal Dec 29, 2017 •

edited

Loading

cjcenizal Jan 5, 2018 •

edited

Loading

cjcenizal Jan 5, 2018 •

edited

Loading

cjcenizal commented Jan 5, 2018 •

edited

Loading

uboness Jan 23, 2018 •

edited

Loading

uboness commented Jan 23, 2018 •

edited

Loading

cjcenizal left a comment •

edited

Loading