Reactify visualize listing table #14227

stacey-gammon · 2017-09-29T23:17:53Z

Third times a try?

Let see if we can come to an agreement. :)

I have to add in another table similar to the dashboard, visualization and management tables, so decided to pick this back up again.

Tried to start with a very non-refactored, stateless version in the ui framework. Only migrated visualizations over, though dashboards would be next.

@cjcenizal - before I take this much further, would you mind, when you get a chance, to take a look at the listing table component and lmk what you think? I can also move it out of the uiframework and into ui/public/listingtable if you think it doesn't belong in the ui framework. Or we can discuss again how best to refactor/unrefactor.

@chrisronline - I heard you have been struggling with trying to find the right way to refactor these, or similar, table elements with sorting, paging, selecting. Lets get together some time next week and share notes! Would love to see this problem solved, one way or another.

Oh and P.S. -- this isn't a final version, just a start. There would still be a ton of duplicated code in vis & dash (and others) listing pages.

cjcenizal · 2017-09-29T23:59:12Z

@stacey-gammon Nice! I like this approach so far. I think this is a great starting point because the abstraction hasn't really overcommitted in any particular direction. Everything that's abstracted looks reasonable, and the person using the ListingTable component has control over things like pagination and table actions.

I think from here we have some interesting options in terms of how to abstract further -- I'm down to talk about it if you want, or feel free to just ping me for more reviews.

cjcenizal · 2017-09-29T23:59:48Z

I think it's OK to leave this in the UI Framework too, just please prefix the React components with "Kui", e.g. KuiListingTable.

stacey-gammon · 2017-10-03T14:59:02Z

@cjcenizal - what do you think about the doc site. Should I change ControlledTable examples to ListingTable examples? Or create duplicates? Visually, they look almost exactly the same, just the code differs.

I'm leaning towards switching it to ListingTable. A benefit of the ListingTable is that the checkbox selection logic works and the footer # of items selected updates naturally.

cjcenizal · 2017-10-03T15:37:33Z

@stacey-gammon Sounds good to me!

cjcenizal

🔔 This is really great, Stacey! This is nicely abstracted, it's pretty easy to understand, I feel like the consumer still has a lot of control over the table, and it clearly reduces the amount of code to create a listing page.

I left a lot of comments but the majority of them are nitpicky ones. I only left one or two which are significant but I think they're in line with the direction you've established here. Let me know what you think!

cjcenizal · 2017-10-04T23:05:09Z

src/core_plugins/kibana/public/visualize/listing/visualize_listing_table.js

+    window.location = '#/visualize/new';
+  }
+
+  renderToolbarActions() {


For consistency, could you do a find/replace and change all references of "Toolbar" to "ToolBar" and "toolbar" to "toolBar"?

cjcenizal · 2017-10-04T23:59:00Z

ui_framework/doc_site/src/views/table/listing_table.js

+  RIGHT_ALIGNMENT
+} from '../../../../services';
+
+function wrapInRowCell(cell, key, alignment) {


Minor, but you could set a default value of alignment = LEFT_ALIGNMENT which would eliminate the need for the ternary expression.

cjcenizal · 2017-10-05T00:06:14Z

ui_framework/src/components/table/listing_table/listing_table.js

+
+KuiListingTable.PropTypes = {
+  columns: PropTypes.arrayOf(PropTypes.node),
+  rows: PropTypes.arrayOf(PropTypes.shape({


I think we should add defaultProps for both rows and selectedRowIds and set these to an empty array [] by default. This way the logic which refers to their length property won't throw an error if these aren't provided. I think this makes sense because from a consumer's standpoint it should be valid to not provide these values if there are no rows or if none are selected.

cjcenizal · 2017-10-05T00:15:02Z

ui_framework/doc_site/src/views/table/listing_table.js

+  }
+
+  renderToolbarActions() {
+    return [


I think we need a key prop on each of these elements. I think this goes for the other examples as well.

cjcenizal · 2017-10-05T00:15:20Z

ui_framework/doc_site/src/views/table/listing_table_loading_items.js

+
+  renderToolbarActions() {
+    return [
+      <KuiButton


key is needed for these buttons.

cjcenizal · 2017-10-05T02:19:24Z

ui_framework/src/components/table/listing_table/listing_table_toolbar.js

+
+KuiListingTableToolbar.propTypes = {
+  onFilter: PropTypes.func.isRequired,
+  pagerComponent: PropTypes.node,


I think we can drop the "Component" from names and just make this pager and actionComponent can become actions. Otherwise we're sort of implementing Hungarian notation.

cjcenizal · 2017-10-05T02:20:41Z

ui_framework/src/components/table/listing_table/listing_table_toolbar_footer.js

+    <KuiToolBarFooter>
+      <KuiToolBarFooterSection>
+        <KuiToolBarText>
+          {itemsSelectedCount > 0 && `${itemsSelectedCount} items selected`}


Can we update this logic so that if itemsSelectedCount is 1 then the noun is singular ("item")?

cjcenizal · 2017-10-05T02:20:58Z

ui_framework/src/components/table/listing_table/listing_table_toolbar_footer.js

+}
+
+KuiListingTableToolbarFooter.PropTypes = {
+  pagerComponent: PropTypes.node,


I think this can just be pager, per an earlier comment.

cjcenizal · 2017-10-05T02:28:02Z

src/core_plugins/kibana/public/visualize/listing/visualize_listing_table.js

+import PropTypes from 'prop-types';
+
+import { SortableProperties } from 'ui_framework/services';
+import { Pager } from 'ui/pager';


We should probably migrate this into the UI Framework. Probably not necessarily as part of this PR though.

cjcenizal · 2017-10-05T02:36:52Z

src/core_plugins/kibana/public/visualize/listing/visualize_listing_table.js

+          columns={this.renderColumns()}
+          onFilter={this.fetchItems}
+          filter={this.state.filter}
+          noItemsPrompt={<NoVisualizationsPrompt />}


How about we combine noItemsPrompt and loading into a single prompt prop, and offload the logic for determining when to show which into the consumer? If a prompt is specified, it will be presented instead of the table. This gives the consumer some more control over exactly what the prompt looks like and what its content is.

cjcenizal · 2017-10-05T16:27:17Z

src/core_plugins/kibana/public/visualize/listing/visualize_listing_table.js

+    );
+  }
+
+  renderPrompt() {


BTW our style recommends early exiting so this would become:

renderPrompt() { if (this.state.isFetchingItems) { return <KuiListingTableLoadingPrompt />; } if (this.items.length === 0) { if (this.state.filter) { return <KuiListingTableNoMatchesPrompt />; } return <NoVisualizationsPrompt />; } return null; }

This makes it a little easier (for me at least) to mentally map conditions to outcome.

isChecked => isSelected contents => content refactor KuiTableRowCell internally

stacey-gammon · 2017-10-09T13:37:25Z

I think this should be ready for another look @cjcenizal, with all changes addressed.

…/listing-tables

cjcenizal

👏 Awesome work! I found a few minor nitpicky things but after they're addressed I'm 👍 to merging.

You might want to pass our ideas about the header and rows props by @kjbekkelund to see if he can think of any improvements to make before we merge.

Thanks for letting me help out with this one!

cjcenizal · 2017-10-09T23:18:32Z

ui_framework/src/components/table/listing_table/listing_table.js

+  prompt,
+}) {
+
+  function areAllRowsChecked() {


Can we rename this to areAllRowsSelected for consistency?

cjcenizal · 2017-10-09T23:21:49Z

ui_framework/src/components/table/listing_table/listing_table_tool_bar_footer.js

+      <KuiToolBarFooterSection>
+        <KuiToolBarText>
+          {itemsSelectedCount === 1 && '1 item selected'}
+          {itemsSelectedCount > 1 && `${itemsSelectedCount} items selected`}


This seems a little odd because these two conditions are implicitly mutually exclusive, but the second one will still be evaluated even if the first one is true. How about making it more explicit with a function?

export function KuiListingTableToolBarFooter({ pager, itemsSelectedCount }) { const renderText = () => { if (itemsSelectedCount === 1) { return "1 item selected"; } return `${itemsSelectedCount} items selected`; }; return ( <KuiToolBarFooter> <KuiToolBarFooterSection> <KuiToolBarText> {renderText()} </KuiToolBarText> </KuiToolBarFooterSection> <KuiToolBarFooterSection> {pager} </KuiToolBarFooterSection> </KuiToolBarFooter> ); }

Yep, getting too fancy! :) It's more readable your way, will update.

cjcenizal · 2017-10-09T23:23:57Z

src/core_plugins/kibana/public/visualize/listing/visualize_listing_table.js

+  KuiListingTableLoadingPrompt
+} from 'ui_framework/components';
+
+export class VisualizeListingTable extends React.Component {


I think the pattern I've been seeing more lately is to import Component from the React module instead.

import React, { Component, } from 'react'; export class VisualizeListingTable extends Component {

cjcenizal · 2017-10-09T23:24:11Z

ui_framework/doc_site/src/views/table/listing_table.js

+  RIGHT_ALIGNMENT
+} from '../../../../services';
+
+export class ListingTable extends React.Component {


cjcenizal · 2017-10-09T23:25:47Z

ui_framework/doc_site/src/views/table/listing_table.js

+    ];
+  }
+
+  renderHeader() {


Does this need to be a method? Can we define this in the constructor instead?

stacey-gammon · 2017-10-11T18:48:44Z

@kjbekkelund or @chrisronline - let me know if you guys want, or have time, to take a glance at this.

chrisronline

Overall, this is looking great! The code is very readable and easy to understand! Love it! ++

I just had a couple high level questions about a couple things!

chrisronline · 2017-10-12T13:52:34Z

src/core_plugins/kibana/public/visualize/listing/visualize_listing_table.js

+          selectedRowIds={this.state.selectedRowIds}
+          rows={this.createRows()}
+          header={this.renderHeader()}
+          onFilter={this.fetchItems}


This seems strange to me. If this is indeed a filter, we shouldn't need to make network requests when it changes, right? We just need to look at the superset of data, and filter it down to some subset?

unfortunately it does require a re-fetch because we don't grab all the items, we only grab the first 100. Eventually we'll implement paging, which will still require a re-fetch.

I'm guessing that's for performance issues? Have we explored doing a scroll with the ES search so we can continue searching in the background while results are available? It might not be a big deal, but it seems like we could show the initial 100, then quickly load the rest behind the scene?

One of our top notch community members explored this but ran into some complications with sorting and our index not having keyword fields for type and description. Read through #11415 to get the background. Something we want to do, but haven't had time to dig in deeper. We closed the issue because as a workaround we have a configurable setting to how many items can be retrieved.

Thanks! Makes sense!

chrisronline · 2017-10-12T14:06:14Z

ui_framework/src/components/table/listing_table/listing_table.js

+}
+
+KuiListingTable.PropTypes = {
+  header: PropTypes.arrayOf(


What's the thinking behind this being data drive, instead of component driven? Like why not construct some component in the caller and pass it in directly?

This is what is passed in now:

return [ { content: 'Title', onSort: this.sortByTitle, isSorted: this.state.sortedColumn === 'title', isSortAscending: this.sortableProperties.isAscendingByName('title'), }, { content: 'Type', onSort: this.sortByType, isSorted: this.state.sortedColumn === 'type', isSortAscending: this.sortableProperties.isAscendingByName('type'), }, ];

But what if we just did:

return [ <KuiTableHeaderCell onSort: this.sortByTitle, isSorted: this.state.sortedColumn === 'title', isSortAscending: this.sortableProperties.isAscendingByName('title') > Title </KuiTableHeaderCell> ];

or something similar?

It feels we have too much unnecessary code around this header concept and a simpler solution might work out better. Thoughts?

This same question/concept applies to maybe some of the other props here, like rows.

I think I understand the inclination to separate the data from the presentational concerns/details, but it feels the coupling is pretty strong already so I'm not sure it's buying us much.

It was that way at first. It changed in this commit- f50f916
after a discussion with @cjcenizal. I like that it's more refactored in the data driven approach. Comment that initiated the change is here: f50f916#r142833708

Yea that makes sense. I'm not sure I love it still, but it's perfectly readable and still easy to understand so carry on!

For context, I've copy/pasted my original comment below. I was hoping that by encapsulating the row, header, and cell components within the table component, it would let the consumer use a simpler interface to specify their content. For example, if the rows' cells just needs to contain text, you can specify each row as an array of strings instead of needing to specify each cell component too. I was also hoping that that this would move some of the sorting logic into KuiListingTable.

That said, I'm not 100% sold on this approach and really welcome people's thoughts on it. I'd love it if we could improve these abstractions, but I'm also OK with reverting it if people find them too difficult to use.

I wonder if there's a way where we can create abstractions for the way headers and rows present data. If the interfaces of these abstractions were to be similar then I think that would be ergonomic for the consumer, since they're very similar components (they're both containers of the same number of cells).

I'm thinking of something like this:

// This component is responsible for instantiating `KuiTableRowCell` instances. // It will iterate over each element in the cells array. You can specify a cell as a string, // an element, a number, or a more complex configuration object. Depending // on how the type is formatted, the values will either be passed as `children` // or as various props to each `KuiTableRowCell`. const rows = this.items.map(item => ( <KuiListingTableRow cells={[ <a className="kuiLink" href={item.link}>item.name</a>, <div className={`kuiIcon kuiIcon--success ${item.icon}`}/>, item.date, { content: item.magnitude, align: 'right', }, ]} /> )); // Same idea here. const header = <KuiListingTableHeader cells={[{ { content: 'Name', sortableId: 'name, // The presence of this property will present a column as sortable }, 'Icon', { content: 'Date', sortableId: 'date, }, { content: 'Magnitude', align: 'right', sortableId: 'magnitude', } }]} /> // And then maybe we could leverage `children' and the table would become this: <KuiListingTable pager={this.renderPager()} toolbarActions={this.renderToolbarActions()} selectedRowIds={this.state.selectedRowIds} header={header} onFilter={() => {}} filter="" onItemSelectionChanged={this.onItemSelectionChanged} > {rows} </KuiListingTable>

What do you think? Does this sound crazy?

Right, as @cjcenizal said, you can still pass an array of nodes or strings for the header and rows, it's just the internal part, not to include the KuiTableRowCell part. But this means we have to pass in sort information and align information sometimes, for the props that go on that outer element.

I'm also flexible if we find a better approach. For now I'll go ahead and submit once this passes, and we can update when/if we find some better abstractions or patterns. Perhaps once we add in an HOC and refactor the sorting logic, we'll find a better way to handle it.

It's easy to follow and totally makes sense. I can't articulate my thoughts against it that well, which could easily indicate they aren't grounded in anything useful. Please continue as is and I'm sure we'll find another time to rethink it and maybe have some more useful conversations (on my side at least!)

…ed, not if there are no rows.

…/listing-tables

chrisronline

LGTM!

kimjoar

This looks good. Should we get some tests in before merging? A fair bit of "complexity" here.

kimjoar · 2017-10-12T15:22:10Z

ui_framework/doc_site/src/views/table/table_example.js

@@ -93,62 +93,62 @@ export default props => (
    </GuideSection>

    <GuideSection
-      title="ControlledTable"
+      title="ListingTable"


Should it also show one with sorting? There's also some mention of innerTable, should we show that here too?

The innerTable isn't it's own component. We could move it out, but for now, it shouldn't get it's own section in the docs view. I will give one of the headers sort attributes.

* Reactify visualize listing table * refactor toolbar => toolBar * use prompt prop to also cover loading and didn't match search logic. * Use default alignment instead of ternary * Add keys to array elements where missing * Add align property to KuiTableHeaderCell * Fix issue with filter not showing up in search tool bar * onCheckChanged => onSelectionChanged * pagerComponent => pager, actionComponent => actions * use singular verbiage when only 1 item is selected * exit early per style guide * fix lint errors * rename columns => header * Refactor KuiTableHeaderCell into KuiListingTable isChecked => isSelected contents => content refactor KuiTableRowCell internally * fix lint errors * areAllRowsChecked => areAllRowsSelected * improve itemSelectedCount logic in KuiListingTableToolBarFooter * React.Component => Component * make header data a variable, not a function * Only consider all rows selected if rows exist and they are all selected, not if there are no rows. * Adding a few KuiListingTable tests * Give one column sort attributes in examples page

stacey-gammon force-pushed the react/listing-tables branch 4 times, most recently from 0908d08 to fca387b Compare October 3, 2017 20:30

stacey-gammon requested review from cjcenizal and chrisronline October 4, 2017 01:19

stacey-gammon added Feature:Dashboard Dashboard related features :Sharing review v6.1.0 labels Oct 4, 2017

Reactify visualize listing table

27ec5ab

stacey-gammon force-pushed the react/listing-tables branch from fca387b to 27ec5ab Compare October 4, 2017 01:26

stacey-gammon added the v7.0.0 label Oct 4, 2017

cjcenizal suggested changes Oct 5, 2017

View reviewed changes

stacey-gammon added 4 commits October 5, 2017 11:30

refactor toolbar => toolBar

96b3e63

use prompt prop to also cover loading and didn't match search logic.

c70f0ac

Use default alignment instead of ternary

b3a9d55

Add keys to array elements where missing

3beb8e6

stacey-gammon force-pushed the react/listing-tables branch from af9a4c2 to 3beb8e6 Compare October 5, 2017 16:24

cjcenizal reviewed Oct 5, 2017

View reviewed changes

stacey-gammon added 6 commits October 5, 2017 13:33

Add align property to KuiTableHeaderCell

4f5417e

Fix issue with filter not showing up in search tool bar

eaab28f

onCheckChanged => onSelectionChanged

55809a7

pagerComponent => pager, actionComponent => actions

cfadb44

use singular verbiage when only 1 item is selected

cdf7369

exit early per style guide

3651c5d

fix lint errors

b52e18d

stacey-gammon force-pushed the react/listing-tables branch from 09e4703 to b52e18d Compare October 5, 2017 17:33

stacey-gammon added 2 commits October 5, 2017 14:31

rename columns => header

83cca1d

Refactor KuiTableHeaderCell into KuiListingTable

f50f916

isChecked => isSelected contents => content refactor KuiTableRowCell internally

stacey-gammon force-pushed the react/listing-tables branch from bcfb278 to f50f916 Compare October 5, 2017 21:00

fix lint errors

c11f928

Merge branch 'master' of https://github.com/elastic/kibana into react…

77db4b7

…/listing-tables

cjcenizal approved these changes Oct 9, 2017

View reviewed changes

stacey-gammon added 4 commits October 11, 2017 10:46

areAllRowsChecked => areAllRowsSelected

8de5e96

improve itemSelectedCount logic in KuiListingTableToolBarFooter

497546f

React.Component => Component

c29c8a6

make header data a variable, not a function

5348022

chrisronline reviewed Oct 12, 2017

View reviewed changes

stacey-gammon added 2 commits October 12, 2017 10:37

Only consider all rows selected if rows exist and they are all select…

381f6fa

…ed, not if there are no rows.

Merge branch 'master' of https://github.com/elastic/kibana into react…

279d89d

…/listing-tables

chrisronline approved these changes Oct 12, 2017

View reviewed changes

kimjoar reviewed Oct 12, 2017

View reviewed changes

stacey-gammon added 2 commits October 12, 2017 12:45

Adding a few KuiListingTable tests

2ec632b

Give one column sort attributes in examples page

2153631

stacey-gammon merged commit c3222ad into elastic:master Oct 12, 2017

stacey-gammon mentioned this pull request Oct 12, 2017

[6.x] Reactify visualize listing table (#14227) #14447

Merged

stacey-gammon deleted the react/listing-tables branch October 24, 2017 13:58

thomasneirynck mentioned this pull request Nov 10, 2017

[WIP] lab section for visualize #14197

Closed

Reactify visualize listing table #14227

Reactify visualize listing table #14227

Conversation

stacey-gammon commented Sep 29, 2017 • edited Loading

cjcenizal commented Sep 29, 2017

cjcenizal commented Sep 29, 2017 • edited Loading

stacey-gammon commented Oct 3, 2017 • edited Loading

cjcenizal commented Oct 3, 2017

cjcenizal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjcenizal Oct 4, 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

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

stacey-gammon commented Oct 9, 2017

cjcenizal 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

stacey-gammon commented Oct 11, 2017

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

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

chrisronline left a comment

Choose a reason for hiding this comment

kimjoar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stacey-gammon commented Sep 29, 2017 •

edited

Loading

cjcenizal commented Sep 29, 2017 •

edited

Loading

stacey-gammon commented Oct 3, 2017 •

edited

Loading

cjcenizal Oct 4, 2017 •

edited

Loading