Add tableLayout prop to EuiTable, EuiBasicTable and EuiInMemoryTable #2697

andreadelrio · 2019-12-18T19:49:25Z

Summary

This PR adds the tableLayout prop to EuiTable which in turns makes it available in EuiBasicTable and EuiInMemoryTable. The default value is fixed with the option to change it to auto.

I thought it'd be good to add a small example to the docs to show the new prop in action.

Fixes #461

Checklist

~~- [ ] Check against all themes for compatability in both light and dark modes~~
- [ ] Checked in mobile

Checked in IE11 and Firefox
Props have proper autodocs
Added documentation examples
Added or updated jest tests
~~- [ ] Checked for breaking changes and labeled appropriately~~
~~- [ ] Checked for accessibility including keyboard-only and screenreader modes~~
A changelog entry exists and is marked appropriately

cchaos

Design wise it mostly looks good, though it seems that truncation doesn't work in auto-layout. Maybe that's ok or expected. But perhaps you could do a quick look into a possible fix?

cchaos · 2019-12-18T20:52:17Z

src/components/table/table.tsx

 } & CommonProps &
  TableHTMLAttributes<HTMLTableElement>;

+const tableLayoutToClassMap: { [tableLayout: string]: string | null } = {
+  fixed: 'euiTable--fixed',


Since you're not actually using this class in the CSS, but the default .euiTable class gets the fixed layout and you're just overriding this with the auto class, you should just set this to null.

Suggested change

fixed: 'euiTable--fixed',

fixed: null,

cchaos · 2019-12-18T20:52:58Z

src/components/table/table.tsx

@@ -5,20 +5,32 @@ import { CommonProps } from '../common';
 export type Props = {
  compressed?: boolean;
  responsive?: boolean;
+  tableLayout?: string;


All new props in TS need a comment explaining what it is. This helps consumers via their IDE's auto-complete. Probably just duplicate what was written in the props_info file.

cchaos · 2019-12-18T20:53:49Z

src/components/table/_table.scss

+  &.euiTable--auto {
+    table-layout: auto;
+  }
+


Nit, extra space here

src/components/basic_table/basic_table.tsx

cchaos · 2019-12-18T21:02:00Z

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

+    return (
+      <div>
+        <EuiSwitch
+          label="Show auto layout"


Suggested change

label="Show auto layout"

label="Auto layout"

cchaos · 2019-12-18T21:03:00Z

src-docs/src/views/tables/auto/auto_section.js

+    <div>
+      <p>
+        <EuiCode>EuiBasicTable</EuiCode> has a fixed layout by default. You can
+        change it to auto using the <EuiCode>tableLayout</EuiCode> prop.


Seems like this description isn't enough to warrant an entire example in the docs? Consider beefing up this explanation or removing the example and just relying on props comments to explain the prop.

Agree that a props comment is likely enough and this concept doesn't need a full example. Only alternative (which should be handled in a separate PR) would be to do something similar to EuiDataGrid where we devote a full section to styling concerns and add an example with lots of toggles.

That was going to be my third suggestion, but figured it out of scope of this PR. So probably best to just remove this example for now.

cchaos · 2019-12-18T21:04:30Z

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

@@ -95,6 +95,14 @@ export const propsInfo = {
          required: false,
          type: { name: '(criteria: #Criteria) => void' },
        },
+        tableLayout: {
+          description: 'Configures table layout',


Probably want to explain that it's configuring the CSS table-layout property.

andreadelrio · 2019-12-19T18:23:50Z

Design wise it mostly looks good, though it seems that truncation doesn't work in auto-layout. Maybe that's ok or expected. But perhaps you could do a quick look into a possible fix?

@cchaos With this I find myself in a loop: to truncate text we would need to set the cell width which would defeat the purpose of setting layout to auto in the first place? Is there any other way to look at it?

I think that when the consumer wants some columns wider than others while still being able to use truncateText they should set the width themselves. So we'd have these three scenarios

Auto layout (truncateText stops working)

Fixed layout (truncateText works)

Fixed layout with First name at 20% width (truncateText works)

If we stick with this, I would add a note in the docs that auto layout overrides truncateText.

cchaos · 2019-12-19T21:51:31Z

If we stick with this, I would add a note in the docs that auto layout overrides truncateText.

I would do that, though it doesn't technically "override" truncateText, but it does inhibit it from calculating the other columns properly. This would then also warrant keeping the docs example so you can also add in this width to your example.

andreadelrio · 2019-12-20T16:18:11Z

If we stick with this, I would add a note in the docs that auto layout overrides truncateText.

I would do that, though it doesn't technically "override" truncateText, but it does inhibit it from calculating the other columns properly. This would then also warrant keeping the docs example so you can also add in this width to your example.

@cchaos I've updated the example to look like this. Let me know what you think

src/components/basic_table/basic_table.tsx

thompsongl · 2019-12-20T17:52:02Z

PR to simplify TypeScript type definitions and extensions: andreadelrio#5

TypeScript cleanup

thompsongl

Code changes LGTM!

cchaos

Looks good, just had a couple more suggestions for the doc example

cchaos · 2019-12-20T19:38:26Z

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

@@ -95,6 +95,15 @@ export const propsInfo = {
          required: false,
          type: { name: '(criteria: #Criteria) => void' },
        },
+        tableLayout: {
+          description:
+            'Sets the table-layout CSS property. Note that auto tableLayout prevents truncateText from working regularly.',


Suggested change

'Sets the table-layout CSS property. Note that auto tableLayout prevents truncateText from working regularly.',

'Sets the table-layout CSS property. Note that auto tableLayout prevents truncateText from working properly.',

cchaos · 2019-12-20T19:48:49Z

src-docs/src/views/tables/auto/auto_section.js

+const html = renderToHtml(Table);
+
+export const section = {
+  title: 'A BasicTable with auto layout',


This section is no longer about just auto layout but layout in general

Suggested change

title: 'A BasicTable with auto layout',

title: 'Table layout',

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

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

src-docs/src/views/tables/auto/auto_section.js

cchaos

Looks great! I think this PR satisfies the issue but pushes towards a better solution (using widths) in the docs.

andreadelrio added 5 commits December 18, 2019 10:46

added auto layout to EuiTable, EuiBasicTable and EuiInMemoryTable

bbfe617

Merge remote-tracking branch 'upstream/master' into auto-table

3cd50e6

removed extra line

8799f73

cl

53b2cdc

fixed cl

8487fe6

andreadelrio requested review from thompsongl and cchaos December 18, 2019 19:54

cchaos suggested changes Dec 18, 2019

View reviewed changes

andreadelrio added 4 commits December 19, 2019 21:27

added custom width to example

aac55b1

updated cl

04b827f

updated cl again

9b79975

revert EuiTableDataType

992e826

updated prop info

45879ae

andreadelrio commented Dec 20, 2019

View reviewed changes

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

ts, cleanup

2ab81df

Merge pull request #5 from thompsongl/auto-table

fa72330

TypeScript cleanup

thompsongl approved these changes Dec 20, 2019

View reviewed changes

cchaos reviewed Dec 20, 2019

View reviewed changes

PR feedback

7b1308a

andreadelrio requested a review from cchaos December 20, 2019 20:31

cchaos approved these changes Dec 20, 2019

View reviewed changes

andreadelrio merged commit e1a7c10 into elastic:master Dec 20, 2019

chandlerprall mentioned this pull request Jan 6, 2020

Upgraded EUI to 18.0.0 elastic/kibana#54042

Merged

snide added the typescript label Apr 17, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tableLayout prop to EuiTable, EuiBasicTable and EuiInMemoryTable #2697

Add tableLayout prop to EuiTable, EuiBasicTable and EuiInMemoryTable #2697

andreadelrio commented Dec 18, 2019

cchaos left a comment

cchaos Dec 18, 2019

cchaos Dec 18, 2019

cchaos Dec 18, 2019

cchaos Dec 18, 2019

cchaos Dec 18, 2019

snide Dec 18, 2019

cchaos Dec 18, 2019

cchaos Dec 18, 2019

andreadelrio commented Dec 19, 2019

cchaos commented Dec 19, 2019

andreadelrio commented Dec 20, 2019

thompsongl commented Dec 20, 2019

thompsongl left a comment

cchaos left a comment

cchaos Dec 20, 2019

cchaos Dec 20, 2019

cchaos left a comment

	'Sets the table-layout CSS property. Note that auto tableLayout prevents truncateText from working regularly.',
	'Sets the table-layout CSS property. Note that auto tableLayout prevents truncateText from working properly.',

	title: 'A BasicTable with auto layout',
	title: 'Table layout',

Add tableLayout prop to EuiTable, EuiBasicTable and EuiInMemoryTable #2697

Add tableLayout prop to EuiTable, EuiBasicTable and EuiInMemoryTable #2697

Conversation

andreadelrio commented Dec 18, 2019

Summary

Checklist

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

Choose a reason for hiding this comment

andreadelrio commented Dec 19, 2019

cchaos commented Dec 19, 2019

andreadelrio commented Dec 20, 2019

thompsongl commented Dec 20, 2019

thompsongl left a 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

cchaos left a comment

Choose a reason for hiding this comment