Add enableSorting prop to EuiBasicTable for default sorting of table. #2906

Anupam-dagar · 2020-02-23T14:05:26Z

Summary

This PR adds an enableSorting prop to EuiBasicTable component. This prop when set to true will make all columns sortable by setting sortable property of each column to true.

Checklist

Check against all themes for compatibility 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

As pointed out by @cchaos in the issue, this PR doesn't yet handle the case if the data in the column isn't what the sorting function accepts. I would implement it as the discussion continues over it in this PR.

kibanamachine · 2020-02-23T14:05:28Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

cchaos · 2020-02-24T15:51:41Z

Jenkins, test this

kibanamachine · 2020-02-24T16:08:52Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_2906/

cchaos

Thanks @Anupam-dagar. Though the PR is not quite complete. I have listed out some more tasks that need to be done. Meanwhile, I'll check for functionality.

cchaos · 2020-02-24T16:07:58Z

src/components/basic_table/basic_table.tsx

@@ -189,6 +189,7 @@ interface BasicTableProps<T> extends Omit<EuiTableProps, 'onChange'> {
  rowProps?: object | RowPropsCallback<T>;
  selection?: EuiTableSelectionType<T>;
  sorting?: EuiTableSortingType<T>;
+  enableSorting?: boolean;


There are a few more things that will need to be done when we add new props.

1. Add a prop docs comment to this so consumers understand what it does. Something like:

Suggested change

enableSorting?: boolean;

/**

* Attempts to enable the default sorting ability for each column

**/

enableSorting?: boolean;

EuiBasicTable is also unique in that it has it's own props docs configuration file buried in the docs site. You'll find that file here, in which you'll need to add another section for this new prop and add a description similar to the comment above.

2. Add a test for this prop in basic_table.test.tsx

You can follow the pattern by copying and pasting this block and changing the configs to test the enableSorting prop.

3. Add a sentence to the table sorting section alluding to this new prop.

This section is where it talks about sorting.

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

Lines 23 to 28 in ca74ad3



The following example shows how to configure column sorting via the{' '}

<EuiCode>sorting</EuiCode>

property and flagging the sortable columns as{' '}

<EuiCode>sortable: true</EuiCode>



I'd change it to read something like:

 The following example shows how to configure column sorting via the{' '} <EuiCode>sorting</EuiCode> property and flagging the sortable columns as{' '} <EuiCode>sortable: true</EuiCode>. You can also attempt to enable the default sorting behavior of all columns by passing <EuiCode>enableSorting: true</EuiCode> to EuiBasicTable without the need to specify it for each individual column. 

4. Add a changelog entry

cchaos · 2020-02-24T16:46:32Z

@Anupam-dagar Have you tested this new option in the docs? I'm not seeing any change to the EuiBasicTable render (none of the table columns have the sorting functionality) when I change the prop to be true.

Anupam-dagar · 2020-02-25T14:15:00Z

@cchaos Thanks for your review!
I tried this on the sorting, pagination and footer table example by adding enableSorting={true} to the props. I was able to see the sorting working. For sorting example, I removed sortable: true from all the columns to see if it is working.
With my understanding of the issue, please correct me if I am wrong, I assume that sort and onchange function will be provided and enableSorting will just make every column sortable.

I have attached the gif below on how I tested it.

src/components/basic_table/basic_table.tsx

Anupam-dagar · 2020-02-26T15:54:01Z

@chandlerprall I have moved the prop to sort prop itself. I kept it optional so that it doesn't break the current sorting implementations. I added the code block inside the field check, I guess sortable can be made true to field only columns since columns having render needs a function for sortable.

src/components/basic_table/table_types.ts

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

Anupam-dagar · 2020-02-26T17:03:26Z

@chandlerprall I have made the requested changes.

chandlerprall

One final change requested.

src/components/basic_table/basic_table.tsx

Anupam-dagar · 2020-02-27T03:37:47Z

@chandlerprall Implemented the requested changes.

chandlerprall

Changes LGTM!; Pulled and tested locally

@cchaos mind taking another pass at the documentation side?

cchaos · 2020-02-27T20:58:33Z

Jenkins, test this

kibanamachine · 2020-02-27T21:17:16Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_2906/

cchaos

I also had a thought to check the mobile view, and so I added the prop to the tables/mobile.js example file, and the desktop view correctly enables the sorting for all columns, but the mobile sort dropdown only lists the columns that have sortable: true directly on them.

cchaos · 2020-02-27T21:01:42Z

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

+      <EuiCode>sortable: true</EuiCode>. You can also set the default sorting
+      behavior of field columns by passing
+      <EuiCode>enableAllColumns: true</EuiCode> to <EuiCode>sorting</EuiCode>
+      prop of EuiBasicTable without the need to specify it for each individual
+      column.


Suggested change

<EuiCode>sortable: true</EuiCode>. You can also set the default sorting

behavior of field columns by passing

<EuiCode>enableAllColumns: true</EuiCode> to <EuiCode>sorting</EuiCode>

prop of EuiBasicTable without the need to specify it for each individual

column.

<EuiCode>sortable: true</EuiCode>. To enable the default sorting ability to every

column, pass <EuiCode>enableAllColumns: true</EuiCode> to the <EuiCode>sorting</EuiCode>

prop.

Anupam-dagar · 2020-02-28T09:04:41Z

@cchaos I have made the changes to make the prop work for the mobile view.

cchaos

Thanks @Anupam-dagar! As a side note, it's really helpful to reviewers if you don't merge down all new commits to the PR. It helps us as reviewers to see the latest change by viewing individual commits. When we merge the PR, then we squash commits so atleast master will only have one commit per PR.

cchaos · 2020-02-28T17:16:37Z

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

@@ -24,7 +24,11 @@ export const section = {
      The following example shows how to configure column sorting via the{' '}
      <EuiCode>sorting</EuiCode>
      property and flagging the sortable columns as{' '}
-      <EuiCode>sortable: true</EuiCode>
+      <EuiCode>sortable: true</EuiCode>.To enable the default sorting ability to


Typo here (my fault)

Suggested change

<EuiCode>sortable: true</EuiCode>.To enable the default sorting ability to

<EuiCode>sortable: true</EuiCode>.To enable the default sorting ability for

Also, when there's a line break and then element tags, it gets parsed as not having any space. You then need to add {' '} as you'll notice in the previous lines. I'd suggest installing a linter that can auto-fix when you save. I know it helps me a lot.

Anupam-dagar · 2020-02-29T13:22:36Z

@cchaos I'll remember to not merge down my commits on my next PR's. I have fixed the typo in the sorting section.

chandlerprall · 2020-02-29T16:00:07Z

jenkins test this

kibanamachine · 2020-02-29T16:18:34Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_2906/

chandlerprall

There is one failing test snapshot that needs updating; please run yarn test-unit -u and commit the result.

chandlerprall · 2020-02-29T16:42:11Z

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

@@ -24,7 +24,11 @@ export const section = {
      The following example shows how to configure column sorting via the{' '}
      <EuiCode>sorting</EuiCode>
      property and flagging the sortable columns as{' '}
-      <EuiCode>sortable: true</EuiCode>
+      <EuiCode>sortable: true</EuiCode>.To enable the default sorting ability


One last formatting issue that I can see

Suggested change

<EuiCode>sortable: true</EuiCode>.To enable the default sorting ability

<EuiCode>sortable: true</EuiCode>. To enable the default sorting ability

Anupam-dagar · 2020-02-29T17:05:15Z

@chandlerprall requested changes done. I hope the PR is now complete.
I had to commit using -n flag since the pre commit test was giving the following error and I guess it is unrelated to the code I have edited. It's from the combo box.

yarn run v1.21.1
$ yarn tsc --noEmit && yarn lint-es && yarn lint-sass
$ /home/anupam/github/eui/node_modules/.bin/tsc --noEmit
src/components/combo_box/combo_box.tsx:651:39 - error TS2339: Property 'copyInputStyles' does not exist on type 'AutosizeInput & HTMLDivElement'.

651         this.autoSizeInputRefInstance.copyInputStyles();
                                          ~~~~~~~~~~~~~~~


Found 1 error.

Fixes: elastic#1866

cchaos · 2020-03-10T14:25:55Z

Jenkins, test this

kibanamachine · 2020-03-10T14:43:18Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_2906/

chandlerprall

Changes LGTM!

cchaos mentioned this pull request Feb 24, 2020

Default sortable to true for all table columns #1866

Closed

cchaos suggested changes Feb 24, 2020

View reviewed changes

cchaos requested a review from chandlerprall February 24, 2020 16:11

chandlerprall suggested changes Feb 25, 2020

View reviewed changes

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

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

Anupam-dagar force-pushed the default_sorting branch from fac4dad to 07bf3f7 Compare February 26, 2020 15:47

chandlerprall suggested changes Feb 26, 2020

View reviewed changes

src/components/basic_table/table_types.ts Outdated Show resolved Hide resolved

src-docs/src/views/tables/sorting/sorting_section.js Outdated Show resolved Hide resolved

src-docs/src/views/tables/sorting/sorting_section.js Outdated Show resolved Hide resolved

Anupam-dagar force-pushed the default_sorting branch from 07bf3f7 to 4894b42 Compare February 26, 2020 17:02

chandlerprall suggested changes Feb 26, 2020

View reviewed changes

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

Anupam-dagar force-pushed the default_sorting branch from 4894b42 to 055ccaa Compare February 27, 2020 03:36

chandlerprall approved these changes Feb 27, 2020

View reviewed changes

cchaos suggested changes Feb 27, 2020

View reviewed changes

Anupam-dagar force-pushed the default_sorting branch 2 times, most recently from d24e4a5 to 8eb439b Compare February 28, 2020 09:02

cchaos approved these changes Feb 28, 2020

View reviewed changes

chandlerprall suggested changes Feb 29, 2020

View reviewed changes

Anupam-dagar force-pushed the default_sorting branch from 0fe58d7 to 37b0d49 Compare February 29, 2020 17:02

Anupam-dagar and others added 4 commits March 1, 2020 09:00

Add enableSorting prop to EuiBasicTable for default sorting of table.

eb851ad

Fixes: elastic#1866

fix props description typo in sorting section docs.

069693f

fix typo and update test snapshot.

37b0d49

Merge branch 'master' into default_sorting

745b8b2

cchaos requested a review from chandlerprall March 10, 2020 14:25

chandlerprall approved these changes Mar 10, 2020

View reviewed changes

chandlerprall merged commit fa2ec10 into elastic:master Mar 10, 2020

chandlerprall mentioned this pull request Mar 16, 2020

Upgrade EUI to 21.0.1 elastic/kibana#60282

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add enableSorting prop to EuiBasicTable for default sorting of table. #2906

Add enableSorting prop to EuiBasicTable for default sorting of table. #2906

Anupam-dagar commented Feb 23, 2020

kibanamachine commented Feb 23, 2020

cchaos commented Feb 24, 2020

kibanamachine commented Feb 24, 2020

cchaos left a comment

cchaos Feb 24, 2020

cchaos commented Feb 24, 2020

Anupam-dagar commented Feb 25, 2020

Anupam-dagar commented Feb 26, 2020

Anupam-dagar commented Feb 26, 2020

chandlerprall left a comment

Anupam-dagar commented Feb 27, 2020

chandlerprall left a comment

cchaos commented Feb 27, 2020

kibanamachine commented Feb 27, 2020

cchaos left a comment

cchaos Feb 27, 2020

Anupam-dagar commented Feb 28, 2020

cchaos left a comment

cchaos Feb 28, 2020

Anupam-dagar commented Feb 29, 2020

chandlerprall commented Feb 29, 2020

kibanamachine commented Feb 29, 2020

chandlerprall left a comment

chandlerprall Feb 29, 2020

Anupam-dagar commented Feb 29, 2020

cchaos commented Mar 10, 2020

kibanamachine commented Mar 10, 2020

chandlerprall left a comment

	<p>
	The following example shows how to configure column sorting via the{' '}
	<EuiCode>sorting</EuiCode>
	property and flagging the sortable columns as{' '}
	<EuiCode>sortable: true</EuiCode>
	</p>

	<EuiCode>sortable: true</EuiCode>.To enable the default sorting ability to
	<EuiCode>sortable: true</EuiCode>.To enable the default sorting ability for

	<EuiCode>sortable: true</EuiCode>.To enable the default sorting ability
	<EuiCode>sortable: true</EuiCode>. To enable the default sorting ability

Add enableSorting prop to EuiBasicTable for default sorting of table. #2906

Add enableSorting prop to EuiBasicTable for default sorting of table. #2906

Conversation

Anupam-dagar commented Feb 23, 2020

Summary

Checklist

kibanamachine commented Feb 23, 2020

cchaos commented Feb 24, 2020

kibanamachine commented Feb 24, 2020

cchaos left a comment

Choose a reason for hiding this comment

cchaos Feb 24, 2020

Choose a reason for hiding this comment

1. Add a prop docs comment to this so consumers understand what it does. Something like:

2. Add a test for this prop in basic_table.test.tsx

3. Add a sentence to the table sorting section alluding to this new prop.

4. Add a changelog entry

cchaos commented Feb 24, 2020

Anupam-dagar commented Feb 25, 2020

Anupam-dagar commented Feb 26, 2020

Anupam-dagar commented Feb 26, 2020

chandlerprall left a comment

Choose a reason for hiding this comment

Anupam-dagar commented Feb 27, 2020

chandlerprall left a comment

Choose a reason for hiding this comment

cchaos commented Feb 27, 2020

kibanamachine commented Feb 27, 2020

cchaos left a comment

Choose a reason for hiding this comment

cchaos Feb 27, 2020

Choose a reason for hiding this comment

Anupam-dagar commented Feb 28, 2020

cchaos left a comment

Choose a reason for hiding this comment

cchaos Feb 28, 2020

Choose a reason for hiding this comment

Anupam-dagar commented Feb 29, 2020

chandlerprall commented Feb 29, 2020

kibanamachine commented Feb 29, 2020

chandlerprall left a comment

Choose a reason for hiding this comment

chandlerprall Feb 29, 2020

Choose a reason for hiding this comment

Anupam-dagar commented Feb 29, 2020

cchaos commented Mar 10, 2020

kibanamachine commented Mar 10, 2020

chandlerprall left a comment

Choose a reason for hiding this comment