Support nodes in EuiBasicTable column headers #1234

cjcenizal · 2018-10-04T20:17:30Z

Summary

Fixes #1002

Looks a bit weird on mobile:

Checklist

This was checked in mobile
This was checked in IE11
This was checked in dark mode
Any props added have proper autodocs
Documentation examples were added
A changelog entry exists and is marked appropriately
This was checked for breaking changes and labeled appropriately
Jest tests were updated or added to match the most common scenarios
This was checked against keyboard-only and screenreader scenarios

…ssed table example.

cjcenizal · 2018-10-04T20:18:40Z

src/components/table/mobile/table_sort_mobile.js

@@ -22,10 +22,6 @@ export class EuiTableSortMobile extends Component {
    };
  }

-  shouldComponentUpdate(nextProps, nextState) {
-    return JSON.stringify(nextProps) !== JSON.stringify(this.props) || JSON.stringify(nextState) !== JSON.stringify(this.state);


Had to remove this due to a circular dependency error.

There might be some other perf. optimisations we can do here (like PureComponent) but probably not necessary for this PR.

Thinking about this again: JSON.stringify can be fairly expensive, so this change might actually improve perf 🤔

Good point! Ironic. 😄 I wasn't sure if PureComponent was a good choice since it does a shallow compare and we might run into some issues with items but we can investigate later.

Yeah React recommends against using JSON.stringify in shouldComponentUpdate anyway:

We do not recommend doing deep equality checks or using JSON.stringify() in shouldComponentUpdate(). It is very inefficient and will harm performance.

https://reactjs.org/docs/react-component.html#shouldcomponentupdate

sorenlouv

lgtm!

sorenlouv · 2018-10-05T07:19:29Z

src-docs/src/views/tables/sorting/sorting.js

@@ -100,27 +103,68 @@ export class Table extends Component {
    }, {
      field: 'github',
      name: 'Github',


I guess you meant to remove this?

Remove what?

The name property (name: 'Github',)

sorenlouv · 2018-10-05T07:43:37Z

src/components/table/mobile/table_sort_mobile.js

@@ -22,10 +22,6 @@ export class EuiTableSortMobile extends Component {
    };
  }

-  shouldComponentUpdate(nextProps, nextState) {
-    return JSON.stringify(nextProps) !== JSON.stringify(this.props) || JSON.stringify(nextState) !== JSON.stringify(this.state);


There might be some other perf. optimisations we can do here (like PureComponent) but probably not necessary for this PR.

sorenlouv · 2018-10-05T07:44:22Z

src/components/table/mobile/table_sort_mobile.js

@@ -22,10 +22,6 @@ export class EuiTableSortMobile extends Component {
    };
  }

-  shouldComponentUpdate(nextProps, nextState) {
-    return JSON.stringify(nextProps) !== JSON.stringify(this.props) || JSON.stringify(nextState) !== JSON.stringify(this.state);


Thinking about this again: JSON.stringify can be fairly expensive, so this change might actually improve perf 🤔

snide · 2018-10-05T07:50:35Z

src-docs/src/views/tables/sorting/sorting.js

@@ -100,27 +103,68 @@ export class Table extends Component {
    }, {
      field: 'github',
      name: 'Github',
+      name: (
+        <EuiFlexGroup gutterSize="xs" alignItems="center">


Add responsive={false} to fix in mobile

cchaos

I played around with some different ways to add this particular functionality (tooltip to give more info on header) and my feeling is that only having the more tooltip on the icon is both causing rendering issues (mobile) and functional issues (truncation can cause it to not be visible). My solution is to just wrap the entire contents of the header in a tooltip and just append a small, subdued question icon mainly as an indicator.

name: (
  <EuiToolTip content="Colloquially known as a 'birthday'">
    <span>
      Date of Birth <EuiIcon size="s" color="subdued" type="questionInCircle" />
    </span>
  </EuiToolTip>
),

This means if the header is truncated, you'll still get the tooltip:

And in mobile, it's inline:

Side notes

the vertical alignment is not great on small icons, but that's something we need to fix with icons themselves
truncation isn't great when it ends in an icon as the ellipses can be on top of the icon, but that's not easily fixed (unless someone knows how)

cjcenizal · 2018-10-05T13:51:30Z

@cchaos I really like your suggestion! I'll implement this.

cjcenizal · 2018-10-05T14:00:32Z

@cchaos Done:

cchaos · 2018-10-05T14:25:05Z

CHANGELOG.md

@@ -1,6 +1,6 @@
 ## [`master`](https://github.com/elastic/eui/tree/master)

-No public interface changes since `4.4.1`.
+- `EuiBasicTable` accepts nodes as column headers, for supporting things like tooltips and localized text ([#1234](https://github.com/elastic/eui/pull/1234))


Can you re-word to be past tense?

Not sure what you mean, could you give me an example?

Adds support for nodes as column headers in EuiBasicTable for upporting things like tooltips and localized text.

Added support for nodes as column headers in EuiBasicTable for upporting things like tooltips and localized text.

Haha thanks, will update.

cchaos · 2018-10-05T14:29:39Z

src-docs/src/views/tables/custom/custom.js

@@ -455,7 +455,6 @@ export default class extends Component {
          return (
            <EuiTableRowCell
              key={column.id}
-              header={column.label}


Is there a reason why you removed all these?

Sorry I should have explained this in the PR description. I came across this and noticed that it's not the correct attribute name (it should be headers). But after some more digging it looks like this is not a recommend practice and isn't even necessary for accessibility. VoiceOver reads the header value for each cell just fine. So after chatting with @lukeelmers we decided to just remove them.

EuiTableRowCell receives the header prop for displaying the table header inline for mobile. It could probably use a better name (this was one of my early foray's into React). But it is necessary.

eui/src/components/table/table_row_cell.js

Lines 87 to 90 in 0cf8fde

/**

* The column's header title for use in mobile view (will be added as a data-attr)

*/

header: PropTypes.string,

EuiTableRowCell receives the header prop for displaying the table header inline for mobile.

Ahhhh that makes much more sense then! @cjcenizal and I were going back and forth on this and assumed it was intended to be an actual headers attribute and was actually a typo.

It could probably use a better name

+1 -- I think that could help prevent confusion in the future

Yup what Luke said! I'll put it back in the meantime and add it to the propTypes definition if it's not already there.

src/components/basic_table/basic_table.js

cjcenizal · 2018-10-06T17:10:21Z

Thanks for the guidance @cchaos. I put the header prop back. Could you take a look and make sure I didn't miss anything? I also noticed that it was already defined as a propType, but I missed that the first time around somehow.

sorenlouv · 2018-10-10T14:47:27Z

🎉
Thanks @cjcenizal!

cjcenizal added 2 commits October 4, 2018 12:39

Remove unnecessary header props from tables, and delete unused compre…

9f46aa7

…ssed table example.

Add ability to specify nodes as EuiBasicTable headers.

ba692cf

cjcenizal requested review from sorenlouv and chandlerprall October 4, 2018 20:17

Changelog

826410a

cjcenizal commented Oct 4, 2018

View reviewed changes

cjcenizal requested a review from cchaos October 4, 2018 20:19

This was referenced Oct 4, 2018

[Rollups] Rollup support in Kibana, phase 1 elastic/kibana#21117

Merged

[Rollups] Fix i18n bugs elastic/kibana#23848

Merged

sorenlouv approved these changes Oct 5, 2018

View reviewed changes

snide reviewed Oct 5, 2018

View reviewed changes

cchaos suggested changes Oct 5, 2018

View reviewed changes

Caroline and her good ideas.

355e8ef

cchaos reviewed Oct 5, 2018

View reviewed changes

lukeelmers approved these changes Oct 5, 2018

View reviewed changes

Add back header props to table rows.

3dfbe72

Update changelog to be past tense.

13103a3

cchaos approved these changes Oct 10, 2018

View reviewed changes

cjcenizal merged commit aa50e32 into elastic:master Oct 10, 2018

cjcenizal deleted the table-header-node-type branch October 10, 2018 14:40

cjcenizal mentioned this pull request Nov 15, 2018

Convert table cell header into undefined if it's been provided to EuiBasicTable as a non-string node. #1312

Merged

cchaos mentioned this pull request May 7, 2019

[Enhancement] Select all rows, Select all column, Select all cells widget #1226

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support nodes in EuiBasicTable column headers #1234

Support nodes in EuiBasicTable column headers #1234

cjcenizal commented Oct 4, 2018 •

edited

Loading

cjcenizal Oct 4, 2018

sorenlouv Oct 5, 2018

sorenlouv Oct 5, 2018 •

edited

Loading

cjcenizal Oct 5, 2018

lukeelmers Oct 5, 2018

sorenlouv left a comment

sorenlouv Oct 5, 2018

cjcenizal Oct 5, 2018

sorenlouv Oct 5, 2018

sorenlouv Oct 5, 2018

sorenlouv Oct 5, 2018 •

edited

Loading

snide Oct 5, 2018

cchaos left a comment •

edited

Loading

cjcenizal commented Oct 5, 2018

cjcenizal commented Oct 5, 2018

cchaos Oct 5, 2018

cjcenizal Oct 5, 2018

cchaos Oct 5, 2018

cchaos Oct 5, 2018

cchaos Oct 5, 2018

cjcenizal Oct 6, 2018

cchaos Oct 5, 2018

cjcenizal Oct 5, 2018

cchaos Oct 5, 2018

cchaos Oct 5, 2018 •

edited

Loading

lukeelmers Oct 5, 2018

cjcenizal Oct 6, 2018 •

edited

Loading

cjcenizal commented Oct 6, 2018

sorenlouv commented Oct 10, 2018 •

edited

Loading

	/**
	* The column's header title for use in mobile view (will be added as a data-attr)
	*/
	header: PropTypes.string,

Support nodes in EuiBasicTable column headers #1234

Support nodes in EuiBasicTable column headers #1234

Conversation

cjcenizal commented Oct 4, 2018 • edited Loading

Summary

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sorenlouv Oct 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

sorenlouv 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

sorenlouv Oct 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cchaos left a comment • edited Loading

Choose a reason for hiding this comment

cjcenizal commented Oct 5, 2018

cjcenizal commented Oct 5, 2018

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

cchaos Oct 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjcenizal Oct 6, 2018 • edited Loading

Choose a reason for hiding this comment

cjcenizal commented Oct 6, 2018

sorenlouv commented Oct 10, 2018 • edited Loading

cjcenizal commented Oct 4, 2018 •

edited

Loading

sorenlouv Oct 5, 2018 •

edited

Loading

sorenlouv Oct 5, 2018 •

edited

Loading

cchaos left a comment •

edited

Loading

cchaos Oct 5, 2018 •

edited

Loading

cjcenizal Oct 6, 2018 •

edited

Loading

sorenlouv commented Oct 10, 2018 •

edited

Loading