Reactify stateless table components #12349

stacey-gammon · 2017-06-14T19:46:12Z

Reactifies the stateless table components. A next step would be to start incorporating/testing stateful components with logic such as sorting and toggling rows.

I didn't migrate over table.html and table.js because it displays the sort controls with some logic and I didn't want to include any logic in this first migration step.

There is a lot of code added, but it's all very basic. A couple areas that deserve attention are when I'm passing in custom classNames, such as:

<KuiTableRowCell className="kuiTableRowCell--alignRight">
<KuiTableRowCell className="kuiTableRowCell--checkBox">
<KuiTable className="kuiTable--fluid">

We might want to think of making these options on the component so we don't have to pass the actual className. E.g. alignment="right", or alignRight= "true", or isFluid="true" isCheckBox="true".

I don't have a strong opinion on how to structure these props e.g isX=true vs rowType=X, so will wait to hear feedback.

stacey-gammon · 2017-06-14T20:52:33Z

Fixes #12150

cjcenizal

Excellent! This is a big first step.

In terms of modifiers, I'd suggest the following, but I'd also like to hear @weltenwort's and @kjbekkelund's thoughts:

// Applies kuiTableRowCell--alignRight.
<KuiTableRowCell align="right" />

// Composes `<KuiTableRowCell className="kuiTableRowCell--checkBox">` and a checkbox input.
<KuiTableRowCheckBoxCell
  isChecked={this.state.isRowSelected}
  onChange={this.onRowSelectionChange}
/>

// Applies kuiTable--fluid (I picked a poor modifier name but we shouldn't change it now for BWC).
<KuiTable shrinkToContent />

cjcenizal · 2017-06-27T19:09:37Z

ui_framework/components/table/table_row_cell_liner.js

+};
+
+
+


Does your IDE support a setting for ensuring that the file ends with a single newline?

It would if it was officially one of our eslint rules :)

cjcenizal · 2017-06-27T19:17:39Z

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

+    );
+  }
+
+  getTableRows() {


What do you think of using renderTableRows, renderPager, etc? I know we've used get in some other places, but I think I'm used to seeing get and set used mostly with getters and setters. render seems more appropriate to me because it indicates a clear connection to the render() method.

cjcenizal · 2017-06-27T19:19:32Z

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

+} from '../../../../components';
+
+export class ControlledTable extends React.Component {
+  getSampleRowData() {


Can we store this on the instance instead?

constructor() { super(); this.rows = [ // data... ]; }

cjcenizal · 2017-06-27T19:21:04Z

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

+        </KuiToolBar>
+
+        <KuiTable>
+          <thead>


What do you think of making a <KuiTableHeader> component to encapsulate this element (and the <tr> element)? This way, if we need to add classes to these elements, that can just be done within the component and it will update everywhere.

cjcenizal · 2017-06-27T19:22:43Z

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

+            </tr>
+          </thead>
+
+          <tbody>


Likewise with <KuiTableBody>.

cjcenizal · 2017-06-27T19:23:28Z

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

+    return this.getSampleRowData().map(rowData => {
+      return (
+        <KuiTableRow>
+        <KuiTableRowCell className="kuiTableRowCell--checkBox">


Looks like we need to indent everything from this line to line 94 once.

cjcenizal · 2017-06-27T19:26:24Z

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

+            <input type="checkbox" className="kuiCheckBox" />
+          </KuiTableRowCellLiner>
+        </KuiTableRowCell>
+        <KuiTableRowCell>


Can we run the rowData array through another map to generate the cells, and conditionally apply alignRight to the last one? This will reduce a lot of the JSX here, simplifying it a bit I think.

weltenwort · 2017-06-29T16:36:00Z

I really like your suggestions, @cjcenizal, and wonder if we can derive some rules for choosing either. Something like

<KuiTableRowCell align="right" /> for choices between multiple values for the same property (that are not boolean).
<KuiTableRowCheckBoxCell isChecked={/* ... */} /> for modifiers that influence the applicable properties or that are mutually exclusive (but not the same property, see above). Otherwise the set of properties would be the union of all variant's properties.

I'm not certain about <KuiTable shrinkToContent />, since boolean properties that are not expected to be toggled at runtime (in contrast to something like isChecked={/* ... */}) are to be avoided according to clean code conventions. Something of the likes of <KuiFluidTable /> might be more appropriate. I would be interested to hear other opinions on that.

cjcenizal

Just had a couple more suggestions, some nitpicky, some not. 😄

cjcenizal · 2017-06-29T17:26:11Z

ui_framework/components/index.js

+  KuiTableHeaderCell,
+  KuiTableRow,
+  KuiTableRowCell,
+  KuiTableRowCellLiner,


I think we can get away with removing this component and just hardcoding the <div> and class inside of the render method of KuiTableRowCell. What do you think?

I was hoping you'd say something like that! Yes, if we can get away with that, I agree it makes more sense nested and less as an actual component. I wasn't quite sure what the point of it was and if it was only going to be used sometimes. Sounds like it's used all the time for table row cells, so I'll embed it.

cjcenizal · 2017-06-29T17:57:11Z

ui_framework/components/table/table_row.test.js

+} from './table_row_cell';
+
+test('renders KuiTableRow', () => {
+  const component = <KuiTableRow { ...requiredProps }><KuiTableRowCell>hi</KuiTableRowCell></KuiTableRow>;


Can we indent this for better readability?

const component = ( <KuiTableRow { ...requiredProps }> <KuiTableRowCell>hi</KuiTableRowCell> </KuiTableRow> );

Same with this, we should have style rules for this type of thing. Maybe something like, don't put nested tags on the same line.

Sure, I can add this to the HTML style guide.

cjcenizal · 2017-06-29T17:58:08Z

ui_framework/components/table/table_row_check_box_cell.js

+  const classes = classNames('kuiTableRowCell--checkBox', className);
+  return <KuiTableRowCell className={ classes } {...rest} >
+    <KuiTableRowCellLiner>
+      <input type="checkbox" className="kuiCheckBox" onChange={ onChange } checked={ isChecked } />


Can we put these props on individual lines?

<input type="checkbox" className="kuiCheckBox" onChange={ onChange } checked={ isChecked } />

Do we have a style rule for this kind of thing? It'd be easier to know what the rule is so I can get it right the first time, because I generally prefer fewer lines if it fits within the per line char limit which I believe is 120. Whether the rule is something like "if there are x number or greater props, put them individual lines", or "if your props go over x characters, put them on individual lines". Knowing this will save both of us time, especially as more developers start creating react code. Unless there is a hard rule, I think it will be impossible to keep up with these kind of review comments.

I'd just apply the rules from the HTML style guide to our JSX, because they're both declarative "markup" with similar rules. There's a rule in there about multiple attributes on multiple lines.

Ahhh, I had no idea we had that rule. I thought all this time you were just asking because you personally liked it better that way. Sorry about that! 😸

cjcenizal · 2017-06-29T18:14:35Z

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

+    return this.rows.map(rowData => {
+      return (
+        <KuiTableRow>
+          <KuiTableRowCheckBoxCell isChecked={ this.isItemChecked(rowData) } onChange={ () => this.toggleItem(rowData) }/>


Can we put each prop on its own line per the HTML style guide:

<KuiTableRowCheckBoxCell isChecked={ this.isItemChecked(rowData) } onChange={ () => this.toggleItem(rowData) } />

cjcenizal · 2017-06-29T18:20:38Z

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

+  constructor(props) {
+    super(props);
+    this.state = {
+      selectedItems: []


What would you say to drastically simplifying the selection logic? For example, just storing a Map of whether a row is selected or not:

this.state = { rowToSelectedStateMap: new Map(), }; toggleItem = (item) => { const rowToSelectedStateMap = new Map(this.state.rowToSelectedStateMap); rowToSelectedStateMap.set(item, !rowToSelectedStateMap.get(item)); this.setState({ rowToSelectedStateMap, }); };

Then I was thinking that later on we could create a "TableSandbox" page which implements best practices for searching, pagination, sorting, selection, lazy-loading, etc. That would declutter this view. I also think that the current selection logic here is a bit of a strawman, because we would probably construct our rows using a pure data structure, with an ID per row (which we could use for tracking selected state), instead of using arrays of nodes.

I copied what we are currently doing in the landing pages. Switching it doesn't feel like a clear win to me because it complicates other functions (e.g. areAllItemsChecked would now require looping through every item in the map). What will we want people to do in the real world, like you mention, when we have an id? It sounds like in that case we'd continue doing what we are doing with landing pages and how it is in here now?

I agree on the TableSandbox thing, though ideally I think we would have that logic in a single component so it's tested and re-usable, though I think we differ on our opinions regarding that. If we had a component like that it could be used in this example, and the code here minimized.

So I guess I feel like since this example does not match what we'd do in the real world anywa, because it uses the nodes, not "items", then it's not really worth the time to switch it from one strawman implementation to another. I say we leave it as is and work on a component that takes an array of objects, and a column definition, and builds a controlled table from them that is sortable, checkable, pageable, etc. Though the details of that is for another PR and a much longer discussion! :)

thoughts? If you feel really strongly I can switch it out bc to me it's 6 one half a dozen the other, just feels a bit pointless.

My suggestion above was just a way to implement the existing (admittedly non-production-ready) functionality using the React components, so that we can move forward without being hung up on working out the right implementation. 😄

I think we could have a pretty good discussion about the best way to implement this kind of logic (and then encode it within a component or a service), but I'd rather not block this PR on it. So to me, it makes sense to remove it for now and then implement it after we've worked our way through that discussion.

Remove all the toggle logic so checking the checkboxes doesn't work at all in the uiFramework? I agree neither is the right implementation but I actually would rather be able to check the checkboxes in the example component. Having it do nothing will make it seem broken, IMO.

Chatted offline, for the sake of getting this checked in, I'll remove the toggleAll logic since we can't agree on it. I kept the individual toggle row logic so people can at least see how a row looks checked and unchecked.

cjcenizal · 2017-06-29T18:43:35Z

@weltenwort I really appreciate your thought process here! I agree with everything, except I'm still on the fence WRT to:

I'm not certain about , since boolean properties that are not expected to be toggled at runtime (in contrast to something like isChecked={/* ... */}) are to be avoided according to clean code conventions. Something of the likes of might be more appropriate. I would be interested to hear other opinions on that.

I strongly agree with you about clearly communicating which props should be toggled at runtime, and which shouldn't be. The only hitch is that I've seen situations where sometimes you want to add multiple modifiers onto an element, in a way so that they collaborate.

Maybe we can create wrapper components that compose these modifiers?

<KuiFluidTable>
  <KuiTable />
</KuiFluidTable>

KuiFluidTable will just apply the appropriate class to children. We can keep wrapping as we want to apply additional modifiers:

<KuiFluidTable>
  <KuiFixedHeightTable>
    <KuiBizarroTable>
      <KuiTable />
    </KuiBizarroTable>
  </KuiFixedHeightTable>
</KuiFluidTable>

It's kind of ridiculous, but if it scales and communicates intent clearly, then maybe it's the correct option?

stacey-gammon · 2017-06-29T19:36:44Z

Re: the nested table components, it seems excessive to me. I'd rather go with passing props (or even just saying in those very unique situations, pass your own custom css class for your bizarro table). Wrapping every single css class in a react component seems like taking it too far. Would you also do something like <KuiTallTableRow><KuiTableRow>stuff</KuiTableRow></KuiTallTableRow>? Where do you draw the line? But, I don't really know which is the "right" way to do - that's just my gut feel, so take it or leave it.

weltenwort · 2017-06-30T08:28:42Z

Using wrapper components does not seem appropriate here. To me the example of --fluid in combination with --fixedHeight suggests that multiple short-hand boolean properties might actually be fine. <KuiTable shrinkToContent bizarreSetting> doesn't look too bad (compared to the wrappers 😉). And it would allow to easily turn shrinkToContent into width="100px|fluid|full" without drastic api changes.

cjcenizal · 2017-06-30T20:26:50Z

@weltenwort Sounds good to me! Thanks man.

stacey-gammon · 2017-06-30T20:38:52Z

Given the latest assessment on shrinkToContent (keeping it as a bool prop), I think this PR should be ready for a last look.

cjcenizal

Awesome! I found a bunch of really small things.

cjcenizal · 2017-07-12T14:57:16Z

ui_framework/components/table/table_info.js

+
+export const KuiTableInfo = ({ children, className, ...rest }) => {
+  const classes = classNames('kuiTableInfo', className);
+  return <kuiTableInfo className={classes} {...rest} >{children}</kuiTableInfo>;


This should return <div>, not <kuiTableInfo>. Interestingly, Chrome still renders it just fine. 😄

cjcenizal · 2017-07-12T15:00:25Z

ui_framework/components/table/table_header_check_box_cell.js

+export const KuiTableHeaderCheckBoxCell = ({ onChange, isChecked, className, ...rest }) => {
+  const classes = classNames('kuiTableHeaderCell--checkBox', className);
+  return <KuiTableHeaderCell className={ classes } {...rest} >
+      <input type="checkbox" className="kuiCheckBox" onChange={ onChange } checked={ isChecked } />


Small nit, can we format this on multiple lines?

<input type="checkbox" className="kuiCheckBox" onChange={ onChange } checked={ isChecked } />

cjcenizal · 2017-07-12T15:09:25Z

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

+
+        <KuiTable>
+          <KuiTableHeader>
+            <KuiTableHeaderCheckBoxCell isChecked={ false } onChange={ () => {} }/>


We can make this toggleable if we change this to:

<KuiTableHeaderCheckBoxCell isChecked={ this.isItemChecked('header') } onChange={ () => this.toggleItem('header') } />

cjcenizal · 2017-07-12T15:11:05Z

ui_framework/components/tool_bar/tool_bar_text.js

+import React from 'react';
+import classNames from 'classnames';
+
+export const KuiToolBarText = ({ children, className, ...rest }) => {


Can we add a test for this component?

cjcenizal · 2017-07-12T15:16:15Z

ui_framework/components/table/table_header_check_box_cell.js

+import classNames from 'classnames';
+import { KuiTableHeaderCell } from './table_header_cell';
+
+export const KuiTableHeaderCheckBoxCell = ({ onChange, isChecked, className, ...rest }) => {


Can we add a test file for this component, to verify that custom classes and other attributes can be supplied?

cjcenizal · 2017-07-12T15:23:54Z

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

+export function FluidTable() {
+  return (
+     <KuiTable shrinkToContent={ true }>
+        <thead>


Any reason not to use a <KuiTableHeader> here?

cjcenizal · 2017-07-12T15:24:06Z

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

+          </tr>
+        </thead>
+
+        <tbody>


Or <KuiTableBody> here?

cjcenizal · 2017-07-12T15:24:25Z

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

+export function TableWithMenuButtons() {
+  return (
+     <KuiTable>
+        <thead>


<KuiTableHeader>?

cjcenizal · 2017-07-12T15:24:34Z

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

+          </tr>
+        </thead>
+
+        <tbody>


<KuiTableBody>?

cjcenizal · 2017-07-12T15:26:12Z

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

+
+export function TableWithMenuButtons() {
+  return (
+     <KuiTable>


Nitpick: looks like lines 16 and 17 were indented with triple spaces? Does this happen elsewhere?

- get rid of TableRowCellLiner, embed it as a div inside TableRowCell

keep toggle each row logic so people can actually see what a checked row looks like. And fix a bug with a name

stacey-gammon · 2017-07-17T20:31:12Z

Comments addressed.

Just a comment about the tests. A while back when we were first writing tests, I tried to refactor the basic tests so you'd simply have to add the new component to an array, and it would run through and check the basics (children, common props, matches snapshot). The refactoring wasn't fully supported so I didn't press the issue, but I just want to re-voice my opinion that I think it'd be much preferable to refactor all those tests rather than copy/pasting the files and changing three lines. If we ever want to change something across the board, we're looking at updating a LOT of test files. Probably something like 50 files at this point, each which looks exactly the same minus one word change.

cjcenizal · 2017-07-17T20:33:40Z

@stacey-gammon I like that idea. Do we already have an issue for that?

cjcenizal

LGTM! 🎻

stacey-gammon · 2017-07-17T20:40:47Z

No issue for it @cjcenizal. When I first attempted something like this, it was -1'ed (cc @kjbekkelund ) - #10821 (comment). Though perhaps I just didn't go about it the right way. I'll create an issue so we can move the discussion there and figure out how to avoid all this duplication in a way that we are all on board with.

stacey-gammon added Team:Platform-Design Team Label for Kibana Design Team. Support the Analyze group of plugins. :Sharing review v6.0.0 labels Jun 14, 2017

stacey-gammon mentioned this pull request Jun 14, 2017

Introduce react pager component #11802

Merged

stacey-gammon requested a review from cjcenizal June 27, 2017 18:49

cjcenizal suggested changes Jun 27, 2017

View reviewed changes

cjcenizal suggested changes Jun 29, 2017

View reviewed changes

cjcenizal mentioned this pull request Jun 29, 2017

Update React style guide with component design guidelines #12575

Closed

cjcenizal mentioned this pull request Jun 29, 2017

Update HTML style guide with rule on indenting nested tags #12580

Closed

cjcenizal suggested changes Jul 12, 2017

View reviewed changes

stacey-gammon added 6 commits July 17, 2017 15:58

Reactify stateless table components

82a1f5a

address code comments

4654039

Get rid of TableRowCellLiner and add more newlines

48c0fe2

- get rid of TableRowCellLiner, embed it as a div inside TableRowCell

remove obsolete snapshot

aae5dff

Remove toggle all logic

0e09394

keep toggle each row logic so people can actually see what a checked row looks like. And fix a bug with a name

Address code comments

0fa20d1

stacey-gammon force-pushed the uiFramework/stateless-react-table-components branch from 01df113 to 0fa20d1 Compare July 17, 2017 20:31

cjcenizal approved these changes Jul 17, 2017

View reviewed changes

stacey-gammon merged commit 0d18ca4 into elastic:master Jul 18, 2017

This was referenced Jul 18, 2017

[UI Framework] Reactify a few KUI Table CSS Components #12150

Closed

[UI Framework] Reactify components #12351

Closed

jimgoodwin added the release_note:enhancement label Jul 25, 2017

jimgoodwin added the v6.0.0-rc1 label Sep 28, 2017

stacey-gammon deleted the uiFramework/stateless-react-table-components branch October 24, 2017 13:56

stacey-gammon restored the uiFramework/stateless-react-table-components branch October 24, 2017 13:56

stacey-gammon deleted the uiFramework/stateless-react-table-components branch September 21, 2018 14:45

Reactify stateless table components #12349

Reactify stateless table components #12349

Conversation

stacey-gammon commented Jun 14, 2017

stacey-gammon commented Jun 14, 2017

cjcenizal left a comment • 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

weltenwort commented Jun 29, 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

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 Jun 29, 2017 • edited Loading

stacey-gammon commented Jun 29, 2017

weltenwort commented Jun 30, 2017 • edited Loading

cjcenizal commented Jun 30, 2017

stacey-gammon commented Jun 30, 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

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 Jul 17, 2017

cjcenizal commented Jul 17, 2017

cjcenizal left a comment

Choose a reason for hiding this comment

stacey-gammon commented Jul 17, 2017

cjcenizal left a comment •

edited

Loading

cjcenizal commented Jun 29, 2017 •

edited

Loading

weltenwort commented Jun 30, 2017 •

edited

Loading