Introducing EuiBasicTable

[uboness]

This replaces EuiTableOfRecords.

A general purpose table that takes care of most rendering aspect and features needed by a table:

• custom column/cell rendering
• pagination
• column sorting
• selection
• row/item level actions

This component is as stateless as it can be... meaning, all state is expected to be managed by the consumer (a separate EuiBasicTableContainer component will be added later on that also manages state)

This is more than just a rename to TableOfRecords. The model of how it is configured changed as well.

[uboness]

@snide can you add your pagination changes you've made in the search PR here?

[cjcenizal]

This looks great overall. The props changes are a big improvement. I had one suggestion and then I think this is good to merge. I'll probably do a pass on the docs and tests afterwards.

[cjcenizal]

Instead of dispatching all criteria, we can dispatch just what's changed, e.g. pagination data in this case: https://github.com/elastic/eui/pull/376/files#diff-cca6a37365f48c17b61c1ad941a710c1R116

This will work because we're delegating responsibility to the consumer for managing state, so the consumer will "know" this is a partial change and can apply it to the state object correctly: https://github.com/elastic/eui/pull/376/files#diff-fd7470512fc4adaf22762c98d7711056R90

Same idea applies to where we're calling props.onChange elsewhere.

[uboness]

yes, we'll then be delegating the merge to the consumer... either way, the state needs to be merged somewhere... so why not make it easy on the consumer and already take care of it? The fact that we're putting storing the state on the consumers, doesn't mean we should push everything to them... I want to make it as simple as possible to consume and I think telling the consumer "hey... we'll always give you the full criteria... also, regarding the linked ref... there's an assumption that the consumer already has a mechanism to automatically handle partial updates - that's true for react state... that's not the case for redux state... you'll need to do the merge in the reducers. But again, even if you still don't need to handle merge yourself in redux... there's an assumption that the consumer always has this option... and even if they do... what's the harm of us already doing it for them? Bottom line, I don't see any benefit of partial updates - it opens up a door for a lot of errors - simplest would be to tell - "you get the whole state - make sure you act on all of it")

[cjcenizal]

I think dispatching partial updates makes the logic easier to follow within the EuiBasicTable itself. Even after reading the code, I still don't know what to expect from criteria. 😄

It also gives the consumer greater control. For example, you'll have the ability to respond to pagination changes differently than sorting changes if you want to. Considering this component is intended to be wrapped, I think we may find benefits in giving the consumer this level of control. OTOH, I think dispatching the full state object may make sense once we're working with a component which wraps this one, and owns that state.

I'm not following the Redux argument... I can't think of a situation in which the owner of this table's state, whether React or Redux, would not have the ability to merge in changes to that state in an additive manner. Here's an example of it being done with just vanilla JS:

onTableChange = ({ pagination, sorting }) => {
  const { pagination: prevPagination, sorting: prevSorting } = this.state;

  // Overwrite old state with new state.
  this.state = {
    pagination: {
      ...prevPagination,
      ...pagination,
    },
    sorting: {
      ...prevSorting,
      ...sorting,
    },
  };
};

[uboness]

re redux... that's the point... you have to do it yourself (it doesn't auto-magically merge state like in react's setState).

Let me put it in another way... when the state of a react component updates, do you get a partial update of the state or do you get a full state and render it as a whole?

The point is, it's actually simpler and easier to deal with the state as a whole rather than dealing with parts of it and be responsible for merging it or not and then mess with different bits and pieces of it. A whole state doesn't leave room for questions and therefore for mistakes - it's simple, as efficient (as the merge will need to happen somewhere), easy to communicate and to go about.

[cjcenizal]

The point is, it's actually simpler and easier to deal with the state as a whole rather than dealing with parts of it and be responsible for merging it or not and then mess with different bits and pieces of it

I agree with you that it's easier to offload state to the child in this case, but I disagree that it's simpler. It adds complexity because the owner is no longer in control of the state, creating two competing sources of truth. This creates situations like the owner "not knowing" what's changed within the onChange callback -- is it pagination or sorting? The only way to find out is to do a comparison between the state you receive from the table and the state you currently have. I'd really prefer to follow the React model of having a clearly-defined single source of truth (the owner component / the store) and make the child component responsible for telling the owner what's changed (and only what's changed).

[uboness]

so basically, according to your rules... who ever stores the state owns it?

[cjcenizal]

Right, from my perspective ownership === storage.

[uboness]

ok.. so redux store owns the state of an application... interesting. you see, ownership for me is a grant to manipulate state - you get the owner privilege because you know what the state should be and how it should look like... regardless of where it's stored. the owner is the authority on that state. storage is, as the name suggests, a mean of storing something - nothing more, nothing less.

I built a table here with a very simple purpose - spare the consumer the overhead of owning this state or caring about how to manage it... that's the whole point of this table - keep it solely functional - you want pagination - you get pagination... just tell me that you want it. There are a few things I still need from you - if I give you a page to load, you'll need to load the data for me... but for the rest, it's on me. You don't need to think about UI design... you don't need to think about UX best practices, what button should be used, when to hide it and when to disable it. All you need to tell me is that you want a button... I'll lay it out for you... I can't execute the action for you, but I'll let you know when it should be executed. You only need to care about the things that are really important to you... not for how the table is implemented or why I choose one state over another.

THIS is why I built this component - it's the owner of its state - no... it doesn't store it... it just defines a contract of how to interact with it - it tells you what it needs from you and it tells what it provides you... that's it. And nothing in here has anything to do with where the state is stored.

so no... almost by definition for me storage !== ownership. (a database doesn't own the data.. it just stores it... if you do it right, it doesn't even know what the data means... it just knows that it needs to store it somewhere and provide a mean to retrieve it. A data store should not manipulate the data it stores... got forbid... only the owner of the data should and it's not the database.

another example (I'm trying...).... when you store things in a redux store.. the redux store doesn't own the state - it is a simple "centralized" storage for the state. now... many different consumers may look at that state - it doesn't mean they're all owning it... it doesn't mean they're all granted a privilege to modify it as they see fit. And another thing... when you register a listener to the store, it's a general listener... you don't get selective notifications about what exactly changed... you get a notification that the state changed and you can get the whole (new) state. It's your responsibility - as the consumer - to look at the state (as a whole) and decide how to act on it... regardless of what just changed or didn't... just the state. So here you go... two different yet consistent perspectives on state, ownership, and storage.

A data store simply serves as the source of truth of what the data looks like at any point in time... not what it should look like.

[kimjoar]

Went through the code and the discussion, and I agree a bit with both of you. If I had written this as a specific feature in an app I was working on I'd have made most of these actions as separate Redux actions, and let the reducers handle the state logic. That has worked great for me before and kept the components clean and easy to work with. However, that's not what we're building here. This is a reusable table we should use in both the Redux and non-Redux world. Finding the right abstraction for this is probably something that will take a bit of learning and experience to get right.

In this case, given the focus of this table, Uri's approach feels like the right approach to me. It puts "less stress" on the consumer, as this is a table that should just work (otherwise it's either a bug or you should be using some other table). Because of that this won't be the perfect choice for building abstractions on top of (which is probably the intention too, as this is supposed to be an opinionated table?).

(Sidenote — and a discussion for another day: I often see people creating way to granular Redux actions, so you need to dispatch three actions to perform what's actually a single "consistent" change.)

Also, criteria is documented and doing some ifs if you have to perform logic based on onChange isn't too bad, I think.

[uboness]

Also, criteria is documented and doing some ifs if you have to perform logic based on onChange isn't too bad, I think.

exactly... you can always compare two states

[snide]

Pagination changes broken out into a separate PR. #380

[cjcenizal]

Thanks for breaking the tie @kjbekkelund! Merging as-is.