Terra Table #177

nagrawal3 · 2017-03-28T22:05:07Z

Summary

Terra Table React Component

Thanks for contributing to Terra.
@cerner/terra

Matt-Butler · 2017-03-30T03:21:41Z

packages/terra-site/src/examples/table/Index.jsx

+import Markdown from 'terra-markdown';
+import ReadMe from 'terra-table/docs/README.md';
+// Component Source
+// eslint-disable-next-line import/no-webpack-loader-syntax, import/first, import/no-unresolved, import/extensions


Instead of disabling just the next-line, eslint allows you to disable blocks of code.

/* eslint-disable import/no-webpack-loader-syntax, import/first, import/no-unresolved, import/extensions */ import TableSrc from '!raw-loader!terra-table/src/Table'; import SingleSelectableRowsSrc from '!raw-loader!terra-table/src/SingleSelectableRows'; import TableHeaderSrc from '!raw-loader!terra-table/src/TableHeader'; .... /* eslint-enable import/no-webpack-loader-syntax, import/first, import/no-unresolved, import/extensions */

Matt-Butler · 2017-03-30T03:24:00Z

packages/terra-table/README.md

+[![NPM version](http://img.shields.io/npm/v/terra-table.svg)](https://www.npmjs.org/package/terra-table)
+[![Build Status](https://travis-ci.org/cerner/terra-core.svg?branch=master)](https://travis-ci.org/cerner/terra-core)
+
+{insert description}


Nit: please update

Matt-Butler · 2017-03-30T03:30:11Z

packages/terra-table/src/terra-table.scss

@@ -0,0 +1,145 @@
+@import './variables';


I think we should consolidate this will Table.scss.

Matt-Butler · 2017-03-30T03:34:20Z

packages/terra-table/lib/Table.scss

+}
+
+.terra-Table-sort-indicator {
+  float: right;


bidi

https://github.com/cerner/terra-core/blob/master/packages/terra-mixins/src/Mixins.scss#L304

Matt-Butler · 2017-03-30T03:36:25Z

packages/terra-table/src/Table.scss

+@import './mixins';
+@import './terra-table';
+
+[data-column-min-width='tiny'] {


Can we make these css classes instead of using attribute selectors?

Matt-Butler · 2017-03-30T03:44:02Z

packages/terra-table/src/TableHeaderContent.jsx

+      <div>
+        {display}
+        <span className={ascSortInd}>{iconUp}</span>
+        <span className={descSortInd}>{iconDown}</span>


Instead of updating css visibility, can we render only the sort/icon we need?

Matt-Butler · 2017-03-30T03:46:49Z

packages/terra-table/src/TableRow.jsx

+
+const defaultProps = {
+  isSelected: false,
+  isSelectable: undefined,


I keep it undefined because, if Table is non-selectable, then there is no need to define this attribute for every row.

You can just remove it from the defaultProps altogether. It'll be undefined if not passed in. I don't think you need to default isSelected either, but you should probably add a default for height.

removed the default for isSelectable. Cannot add default for height as it will set the maximum height on the row and by default we are not setting restrictions on height

Matt-Butler · 2017-03-30T03:57:48Z

Selectable table rows are not keyboard accessible.

Matt-Butler · 2017-03-30T03:59:43Z

We probably want to update the cursor to pointer for selectable rows:

bjankord · 2017-03-30T13:41:02Z

packages/terra-table/docs/themeable-variables.md

@@ -0,0 +1,3 @@
+# Themeable Variables
+


If there are no themeable sass variables, this file can be removed.

Added variables in doc c9b2d68#diff-a2f8fe2900b7404b7406ee8301ba4387R4

bjankord · 2017-03-30T13:51:52Z

packages/terra-table/src/TableRowContent.jsx

+  /**
+   * Content to be displayed for the row cell
+   */
+  display: PropTypes.any.isRequired,


Nit: Change this to content. It is easier to reason about what this is an matches with other react components we have.

bjankord · 2017-03-30T13:53:06Z

packages/terra-table/src/_variables.scss

+$terra-table-thead-border-width: 1px !default;
+$terra-table-tr-border-color: $terra-grey-30 !default;
+$terra-table-tr-border-width: 1px !default;
+$terra-table-cell-max-height-tiny: 26px;


The max-height and min-width values should be set in rem units.

nagrawal3 · 2017-03-30T21:54:43Z

Updated the code with changes as per above comments

tbiethman · 2017-03-31T15:17:58Z

packages/terra-table/src/TableRowContent.jsx

+
+  return (
+    <td {...customProps} className={contentClassName}>
+      <div>{content}</div>


Do we need to wrap the content in an extra
?

By default, the minimum height set on the table cell is the minimum height required by the content in the cell. Therefore in order to restrict the maximum height of the cell I have to add extra div tag.

JakeLaCombe · 2017-04-03T16:26:03Z

packages/terra-site/src/examples/table/NoStripedTable.jsx

+      <Table.HeaderContent content={'Column Heading 3'} key={3} minWidth={'large'} />
+    </Table.Header>
+    <Table.Rows>
+      <Table.Row key={0}>


I know that it's not good practice to set the key equal to the index of the array, and this section of code feels like it's doing just that, even though there isn't a map or forEach function being called. Could we generate unique ids for these keys instead of using the numbers 1, 2, and 3? I used shortid to generate it in one of my code changes for example.

We should leave it up to the user to set keys. Setting a random key is actually worse than just using the index since keys help React identify which items have changed, are added, or are removed.
Check this thread for some more information.
facebook/react#1342

Totally agree on that. For this example, it feels too closely like it's setting the key equal to the value of it's array index, which also is not a good practice for assigning keys to react components. I feel like a developer would look at this example, and think that it could use the map function for example, and assign the key equal to the numerical index of that given element.

Ah, I didn't pay enough attention to see that this is an example. +1 to using non-index keys in the example.

changed in commit bb5f8be

Matt-Butler · 2017-04-05T14:38:31Z

packages/terra-site/src/examples/table/NoStripedTable.jsx

+import React from 'react';
+import Table from 'terra-table';
+
+const shortid = require('shortid');


Should consumers be providing these keys rather than us generating them?

Sorry, I know there was some discussion about this before. I just want to understand why we do this.

We are generating the keys in example files only, we are not generating the keys in the code.

I still hesitate doing random key generation, even in examples, because we know this is a pattern we don't want to perpetuate. Can we make the examples slightly more meaningful, allowing us to make use of keys that'd make sense? Something like:

<Table isStriped={false}> <Table.Header> <Table.HeaderContent content={'Name'} key="NAME" minWidth={'small'} /> <Table.HeaderContent content={'Address'} key="ADDRESS" minWidth={'medium'} /> <Table.HeaderContent content={'Phone'} key="PHONE_NUMBER" minWidth={'large'} /> </Table.Header> <Table.Rows> <Table.Row key="PERSON_0"> <Table.RowContent content="John Smith" key="NAME" /> <Table.RowContent content="123 Adams Drive" key="ADDRESS" /> <Table.RowContent content="111-222-3333" key="PHONE_NUMBER" /> </Table.Row> <Table.Row key="PERSON_1"> <Table.RowContent content="Jane Smith" key="NAME" /> <Table.RowContent content="321 Drive Street" key="ADDRESS" /> <Table.RowContent content="111-222-3333" key="PHONE_NUMBER" /> </Table.Row> <Table.Row key="PERSON_2"> <Table.RowContent content="Dave Smith" key="NAME" /> <Table.RowContent content="213 Raymond Road" key="ADDRESS" /> <Table.RowContent content="111-222-3333" key="PHONE_NUMBER" /> </Table.Row> </Table.Rows> </Table>

We want to get the point across that the key structure should match the schema of whatever data structure is being presented.

+1 to Tyler's comment. That can allow us to get rid of the additional dependency.

Let's move this to the war room tomorrow. In my component, I'm finding a need to generate random keys, and so far, this is the best solution I found.

Matt-Butler · 2017-04-05T14:56:48Z

packages/terra-site/package.json

@@ -26,6 +26,8 @@
    "react-dom": "^15.4.2",
    "react-intl": "^2.2.3",
    "react-router": "^3.0.2",
+    "shortid": "^2.2.8",
+    "terra-application": "^0.x",


We are deprecating terra-application. Please remove this line.

ryanthemanuel · 2017-04-06T17:40:45Z

packages/terra-site/package.json

@@ -26,6 +26,7 @@
    "react-dom": "^15.4.2",
    "react-intl": "^2.2.3",
    "react-router": "^3.0.2",
+    "shortid": "^2.2.8",


Have we done some analysis about adding this as a dependency?

bjankord · 2017-04-06T19:26:13Z

packages/terra-table/docs/README.md

+
+<Table isStriped={false}>
+  <Table.Header>
+    <Table.HeaderContent display={'Column Heading 1'} key={1} minWidth={'small'} />


The examples are using a content prop instead of display. Should this be updated to switch display prop to content

Agrawal,Nikhil added 18 commits March 20, 2017 16:27

Initial table changes

dbe5f7d

Adding icon for caret up and down

cf1bdad

Adding description for all the attributes

1044cd9

Adding a test route for table

7adf3e0

adding more test files

8269f62

Adding routes to some more test cases

50139de

Adding couple of more test case

e2cd33e

updating test routes file

4899c5e

Updating some react components

6a61ed7

Fixing listing issues

a7dcff2

Pulling latest changes from the original repo

09b84f4

Adding terra-table to package.json for terra-site

475aa4d

Adding jest test

6e2ea77

Adding night watch test cases

e4dfe86

Adding more changes for night watch

957a2c7

Fixing test case

52e646f

Fixing test for small and tiny screen

87e45a7

Removing additional dependencies

daf91e2

Matt-Butler reviewed Mar 30, 2017

View reviewed changes

Agrawal,Nikhil and others added 3 commits March 30, 2017 00:37

Changes as per comments on pull request

e3e5218

Merge branch 'master' into master

0d7f11a

Pulling changes from master

dca3715

bjankord reviewed Mar 30, 2017

View reviewed changes

Updating as per comments on PR

c9b2d68

nagrawal3 added 2 commits March 30, 2017 16:55

Merge branch 'master' into master

f5a0452

updating version of Terra-icon in package.json

f8c7398

tbiethman reviewed Mar 31, 2017

View reviewed changes

JakeLaCombe reviewed Apr 3, 2017

View reviewed changes

nagrawal3 added 3 commits April 4, 2017 11:57

Updating the key to use shortid as per comment by Jake

bb5f8be

Merge branch 'master' into master

22010e7

changing test examples

5ded2e0

Matt-Butler reviewed Apr 5, 2017

View reviewed changes

removing terra-application from package.json

995d52c

tbiethman approved these changes Apr 6, 2017

View reviewed changes

Merge branch 'master' into master

c6462d3

ryanthemanuel reviewed Apr 6, 2017

View reviewed changes

Removing dependency from shortid node package

e2c613b

bjankord reviewed Apr 6, 2017

View reviewed changes

bjankord and others added 2 commits April 6, 2017 14:35

Merge branch 'master' into master

740ed03

updating readme

2fc5c7e

Matt-Butler mentioned this pull request Apr 6, 2017

Terra demographics banner #191

Closed

bjankord approved these changes Apr 7, 2017

View reviewed changes

ryanthemanuel approved these changes Apr 7, 2017

View reviewed changes

bjankord merged commit 33fc98e into cerner:master Apr 7, 2017

bjankord mentioned this pull request Apr 18, 2017

Migrate terra-table into repo #57

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Terra Table #177

Terra Table #177

nagrawal3 commented Mar 28, 2017 •

edited by Matt-Butler

Loading

Matt-Butler Mar 30, 2017

Matt-Butler Mar 30, 2017

Matt-Butler Mar 30, 2017

Matt-Butler Mar 30, 2017

Matt-Butler Mar 30, 2017

Matt-Butler Mar 30, 2017

Matt-Butler Mar 30, 2017

nagrawal3 Mar 30, 2017

tbiethman Mar 30, 2017 •

edited

Loading

nagrawal3 Mar 30, 2017

Matt-Butler commented Mar 30, 2017

Matt-Butler commented Mar 30, 2017

bjankord Mar 30, 2017

nagrawal3 Mar 30, 2017

bjankord Mar 30, 2017

bjankord Mar 30, 2017

nagrawal3 commented Mar 30, 2017

tbiethman Mar 31, 2017

nagrawal3 Mar 31, 2017

JakeLaCombe Apr 3, 2017

mjhenkes Apr 3, 2017

JakeLaCombe Apr 4, 2017

mjhenkes Apr 4, 2017

nagrawal3 Apr 4, 2017

Matt-Butler Apr 5, 2017

Matt-Butler Apr 5, 2017

nagrawal3 Apr 5, 2017

tbiethman Apr 5, 2017 •

edited

Loading

ryanthemanuel Apr 6, 2017

JakeLaCombe Apr 6, 2017

Matt-Butler Apr 5, 2017

ryanthemanuel Apr 6, 2017

bjankord Apr 6, 2017

Terra Table #177

Terra Table #177

Conversation

nagrawal3 commented Mar 28, 2017 • edited by Matt-Butler Loading

Summary

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

tbiethman Mar 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Matt-Butler commented Mar 30, 2017

Matt-Butler commented Mar 30, 2017

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

nagrawal3 commented Mar 30, 2017

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

tbiethman Apr 5, 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

nagrawal3 commented Mar 28, 2017 •

edited by Matt-Butler

Loading

tbiethman Mar 30, 2017 •

edited

Loading

tbiethman Apr 5, 2017 •

edited

Loading