Add support for table footers #1202

lukeelmers · 2018-09-18T23:33:33Z

Closes #493, which is a prerequisite for elastic/kibana#16639.

Summary

Introduces <EuiTableFooter> and <EuiTableFooterCell> components to render a proper <tfoot> in tables. Also adds support for footers to <EuiBasicTable> via a new footer key in the column definition.

Usage

Given the following column definition in <EuiBasicTable>, a <tfoot> will automatically be added with the specified data:

columns: [
    {
        field: 'name',
        name: 'Name',
        description: 'your name',
        footer: <strong>Name</strong> // you can pass JSX...
    },
    {
        field: 'id',
        name: 'ID',
        footer: 'ID' // ...or a string...
    },
    {
        field: 'age',
        name: 'Age',
        footer: items => <strong>{items.reduce((acc, cur) => acc + cur.age, 0)}</strong>
        // ...or a function that returns the contents. The function is passed the full
        // list of items for a table, in case you want to compute derived data from
        // them (for example, calculating the sum of items in a column as shown above.)
    },
    {
        field: 'avatar',
        name: 'Avatar',
        // No footer is included here, so an empty `<td>` will be rendered to ensure a
        // consistent column count.
    }
]

Notes, Caveats, & Discussion Points

If at least one footer is specified in the column definition, the <tfoot> will be rendered. By default, columns with no footer specified (undefined) will render an empty cell to preserve the table layout. ~~You can force footer: null if you want to override this behavior on a per-column basis.~~ (removed; see comment below)
Forcing a colspan is supported in the <EuiTableFooterCell> component itself, but not within <EuiBasicTable>.
- Do we want/need to support a custom colspan within <EuiBasicTable>? Maybe, but we'll decide whether to add this in the future.
- If so, any thoughts on an intuitive way to expose this? column[i].footerColspan is an option, but feels clumsy. We could also make a footer object (i.e. footer: { render, colspan, ...etc }), but that may overcomplicate things for users. TBD later.
If you pass a function to footer in the column definition, the whole list of available items is provided. Just want to note that this means the basic table implementation of footers has no knowledge of pagination... it just looks at all of the available items. This could be easily changed if we want it to, for example, only receive the current visible items. (addressed; see comment below)
Currently this is not responsive and is simply hidden on smaller screens. Responsive tables are a notoriously tricky UX problem and I honestly can't think of a good/obvious way to handle footers... any ideas? Decided we will not support table footers on mobile for the time being.
Styles are placeholder only; I'm assuming we won't want this to be the final visual treatment 😉 @cchaos is looking into this

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

lukeelmers · 2018-09-18T23:35:02Z

@snide @cchaos @ryankeairns @timroes et al, would appreciate any feedback you have on this!

lukeelmers · 2018-09-19T02:53:37Z

One other thought: I purposely kept the configuration minimal to be consistent with the table header implementation, but as a result there are two edge cases to consider:

Right now this assumes every footer cell is a td (tfoot > tr > td), so in the current implementation it isn't possible to, for example, include a th instead (tfoot > tr > th).
It also assumes that every tfoot only has one tr (consistent with the table headers).

We can definitely address these, it's just a question of importance and how we'd want to expose them.

lukeelmers · 2018-09-19T19:02:15Z

Per conversation with @chandlerprall, to keep things simple I will go ahead and always render a <td> in the footer when using <EuiBasicTable>, as opposed to allowing a user to force footer: null. It doesn't really make sense to support this in the column definition if there's no way to also set a colspan.

For now, if someone wants to set their own colspans or skip rendering a <td>, they can do so using the lower-level <EuiTableFooter> and <EuiTableFooterCell> components.

chandlerprall

Overall approach & design look great! Couple small changes.

chandlerprall · 2018-09-19T19:28:24Z

src-docs/src/views/tables/footer/footer_section.js

+ or more of your columns contains a <EuiCode>footer</EuiCode> definition,
+ the footer area will be visible. By default, columns with no footer specified
+ (undefined) will render an empty cell to preserve the table layout. You
+ can force <EuiCode>footer: null</EuiCode> if you want to override this


update description to remove null specifics

ah totally missed this one. thanks for catching

chandlerprall · 2018-09-19T19:28:58Z

src-docs/src/views/tables/basic/props_info.js

+ footer: {
+ description: `Content to display in the footer beneath this column`,
+ required: false,
+ type: { name: 'string | PropTypes.node | (items) => PropTypes.node' }


Use PropTypes.element instead of PropTypes.node, it's a more restrictive and provides flexibility in the future. JS primitives (string, number, boolean, etc) or an array of primitives are acceptable node types. This would allow us to e.g. disable a column footer if false is passed.

chandlerprall · 2018-09-19T19:30:03Z

src/components/basic_table/basic_table.js

+ render: PropTypes.func, // ((value, record) => PropTypes.node (also see [services/value_renderer] for basic implementations)
+ footer: PropTypes.oneOfType([
+ PropTypes.string,
+ PropTypes.node,


PropTypes.element here too

cchaos

The functionality seems to work well. As mentioned below, I'll do a quick design PR against your fork for you.

I'm fine with making the assumption that all table footer cells should be <td>'s but I think the bigger argument would be for accessibility. I can't seem to find any true guidelines on this. We may end up needing to allow some configuration of this depending on what content they're putting in the footer, whether it's just a repeat of the column name or a summary of the column since both would need to have some level of aria support but in different ways.

cchaos · 2018-09-19T19:16:49Z

src/components/table/table_footer_cell.js

+ * Indicates if the column should not show for mobile users
+ * (typically hidden because a custom mobile header utilizes the column's contents)
+ */
+ hideForMobile: PropTypes.bool,


I don't think we'll ever support footers for mobile the way we are changing the layout for small screens. Go ahead and remove the props hideForMobile and isMobileHeader and any logic surrounding these.

Ah, yes isMobileHeader was there because we need to know not to include an empty <td> for that column, however it would be cleaner to just filter this out of the column definition itself rather than relying on CSS to do the work for me. I'll update accordingly.

cchaos · 2018-09-19T19:20:17Z

src/components/table/table_footer_cell.js

+ <td
+ className={classes}
+ colSpan={colSpan}
+ scope={scope}


scope is not supported on <td> elements

Ah, didn't realize this changed in HTML5. Thanks!

cchaos · 2018-09-19T19:24:03Z

src/components/table/_table.scss

@@ -113,6 +113,23 @@
 @include euiTableCellCheckbox;
 }

+// Must come after .euiTableRowCell selector for border to be removed
+.euiTableFooterCell,


This style works pretty well, though a lot of styles are redundant with the table header. I'll put up a PR against your fork for some cleanup.

cchaos · 2018-09-19T19:27:49Z

src/components/table/_table.scss

@@ -113,6 +113,23 @@
 @include euiTableCellCheckbox;
 }

+// Must come after .euiTableRowCell selector for border to be removed
+.euiTableFooterCell,
+.euiTableFooterRow .euiTableHeaderCellCheckbox {


Is the checkbox styles necessary? It seems that you don't pass the checkbox to the footer.

no not necessary. totally meant to pull this out -- I was temporarily experimenting with checkboxes in the footer and didn't clean up the styles after i was done.

cchaos · 2018-09-19T20:16:31Z

If you pass a function to footer in the column definition, the whole list of available items is provided. Just want to note that this means the basic table implementation of footers has no knowledge of pagination... it just looks at all of the available items. This could be easily changed if we want it to, for example, only receive the current visible items.

Could this be added a prop?

lukeelmers · 2018-09-19T22:08:08Z

Could this be added a prop?

@cchaos Yeah, in looking into it more, I'm realizing this would mostly just be useful with <EuiInMemoryTable>, where you might not already have knowledge of which page you are on as the in-memory table handles that for you (with <EuiBasicTable> you'll already be getting a subset of items when pagination is involved).

Perhaps we could pull pageIndex and pageSize from the pagination prop and pass those as arguments to the footer's render function (or just the whole pagination prop).

e.g.

footer: (items, pagination) => {
  const { pageIndex, pageSize } = pagination;
  const startIndex = pageIndex * pageSize;
  const pageOfItems = items.slice(startIndex, Math.min(startIndex + pageSize, items.length));
  // do something with pageOfItems
}

Thoughts?

cchaos · 2018-09-19T22:20:43Z

I'm not super familiar with how passing functions works. @chandlerprall can you take a look at ^^?

lukeelmers · 2018-09-19T22:26:44Z

@cchaos @chandlerprall Just pushed some updates which I believe cover what we've discussed so far; please take a look whenever you have a sec. I'll wait to resolve the merge conflicts until we add the style updates from @cchaos.

cchaos · 2018-09-20T05:21:08Z

Looks like EuiFooterFooter also needs a jest test file.

cchaos · 2018-09-20T05:31:28Z

I also don't think those bundle.js files be being committed.

chandlerprall

small tweaks on that proptype (sorry!)

about including the pagination information to the footer callback function, let's pass both items and pagination in an object, e.g.

footer: ({items, pagination}) => {
  const { pageIndex, pageSize } = pagination;
  const startIndex = pageIndex * pageSize;
  const pageOfItems = items.slice(startIndex, Math.min(startIndex + pageSize, items.length));
  // do something with pageOfItems
}

this grants the flexibility to add more arguments in the future and letting the callback author pick & choose what values to grab, without caring about argument order.

src-docs/src/views/tables/basic/props_info.js

src/components/basic_table/basic_table.js

lukeelmers · 2018-09-20T15:59:09Z

I also don't think those bundle.js files be being committed.

Thanks, will remove those. (Didn't realize these were built during the release process)

lukeelmers · 2018-09-20T16:01:13Z

about including the pagination information to the footer callback function, let's pass both items and pagination in an object, e.g.

this grants the flexibility to add more arguments in the future and letting the callback author pick & choose what values to grab, without caring about argument order.

I like that idea -- will update.

lukeelmers · 2018-09-20T19:00:18Z

@cchaos @chandlerprall Updates pushed; I think I covered everything on our hit list, but ping me with any other thoughts.

chandlerprall

changes LGTM

cchaos

Thanks!

snide

Don't want to hold up this PR, so feel free to merge.

My only comment after pulling it down is that we should likely show an example of the footer that is a common use case. I don't know that people will use it to replicate the table head very often, but more likely would use it to do summary or agg counts of the table contents? It'd be nice to present it that way... ex: 33 users online, 5 countries...etc.

Often people copy / paste our examples, so just giving some level of inspiration there can help. It wouldn't be the first time someone copied something from us and we wondered how something like "footers that duplicate the head" showed up in prod :)

snide · 2018-09-20T22:31:54Z

Thanks for the changes. Looks good @lukeelmers

lukeelmers requested review from snide, timroes, ryankeairns and cchaos September 18, 2018 23:35

lukeelmers force-pushed the feat/table-footer branch from 4108fc6 to 1889247 Compare September 19, 2018 03:00

chandlerprall self-requested a review September 19, 2018 18:48

chandlerprall suggested changes Sep 19, 2018

View reviewed changes

cchaos suggested changes Sep 19, 2018

View reviewed changes

chandlerprall suggested changes Sep 20, 2018

View reviewed changes

src-docs/src/views/tables/basic/props_info.js Outdated Show resolved Hide resolved

src/components/basic_table/basic_table.js Outdated Show resolved Hide resolved

lukeelmers and others added 10 commits September 20, 2018 12:04

Add <EuiTableFooter> and <EuiTableFooterCell>.

71fe966

Add support for footers to <EuiBasicTable>

2e0b43a

Update docs with table footer usage

eab19a9

Resolve docs merge conflicts

dced495

Add table footer tests

2e85ec4

Resolve CHANGELOG conflicts

ddfa428

Force table footers to always have one <td> per column

353d28a

Remove mobile props & scope from table footer, adjust PropTypes

249a5d4

Table footer styles cleanup

98fdc94

Remove committed docs bundle. Reverts c5f0a26

dc769e0

lukeelmers added 2 commits September 20, 2018 12:49

Change table footer function signature to accept pagination

a38cad9

Add missing table header/footer snapshot tests

416d3ce

lukeelmers force-pushed the feat/table-footer branch from b6de9a3 to 416d3ce Compare September 20, 2018 18:55

chandlerprall approved these changes Sep 20, 2018

View reviewed changes

cchaos approved these changes Sep 20, 2018

View reviewed changes

snide approved these changes Sep 20, 2018

View reviewed changes

lukeelmers added 2 commits September 20, 2018 15:35

Ensure column footer data is not passed to every table cell as a prop

8b6b6f4

Update table footer docs with a better example

efd019b

lukeelmers merged commit f80aee5 into elastic:master Sep 20, 2018

lukeelmers deleted the feat/table-footer branch September 20, 2018 22:34

snide mentioned this pull request Oct 3, 2018

Eui 4.4.1 elastic/kibana#23790

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for table footers #1202

Add support for table footers #1202

lukeelmers commented Sep 18, 2018 •

edited

Loading

lukeelmers commented Sep 18, 2018

lukeelmers commented Sep 19, 2018

lukeelmers commented Sep 19, 2018

chandlerprall left a comment

chandlerprall Sep 19, 2018

lukeelmers Sep 19, 2018

chandlerprall Sep 19, 2018

chandlerprall Sep 19, 2018

cchaos left a comment

cchaos Sep 19, 2018

lukeelmers Sep 19, 2018

cchaos Sep 19, 2018

lukeelmers Sep 19, 2018

cchaos Sep 19, 2018

cchaos Sep 19, 2018

lukeelmers Sep 19, 2018

cchaos commented Sep 19, 2018

lukeelmers commented Sep 19, 2018

cchaos commented Sep 19, 2018

lukeelmers commented Sep 19, 2018

cchaos commented Sep 20, 2018

cchaos commented Sep 20, 2018

chandlerprall left a comment

lukeelmers commented Sep 20, 2018 •

edited

Loading

lukeelmers commented Sep 20, 2018

lukeelmers commented Sep 20, 2018

chandlerprall left a comment

cchaos left a comment

snide left a comment

snide commented Sep 20, 2018

Add support for table footers #1202

Add support for table footers #1202

Conversation

lukeelmers commented Sep 18, 2018 • edited Loading

Summary

Usage

Notes, Caveats, & Discussion Points

Checklist

lukeelmers commented Sep 18, 2018

lukeelmers commented Sep 19, 2018

lukeelmers commented Sep 19, 2018

chandlerprall 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

cchaos 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

cchaos commented Sep 19, 2018

lukeelmers commented Sep 19, 2018

cchaos commented Sep 19, 2018

lukeelmers commented Sep 19, 2018

cchaos commented Sep 20, 2018

cchaos commented Sep 20, 2018

chandlerprall left a comment

Choose a reason for hiding this comment

lukeelmers commented Sep 20, 2018 • edited Loading

lukeelmers commented Sep 20, 2018

lukeelmers commented Sep 20, 2018

chandlerprall left a comment

Choose a reason for hiding this comment

cchaos left a comment

Choose a reason for hiding this comment

snide left a comment

Choose a reason for hiding this comment

snide commented Sep 20, 2018

lukeelmers commented Sep 18, 2018 •

edited

Loading

lukeelmers commented Sep 20, 2018 •

edited

Loading