TypeScriptified basic_table. #2428

sainthkh · 2019-10-15T07:53:18Z

Summary

Related to #1557.
Closes #2523

TypeScriptified basic_table dir. In other words, EuiBasicTable and EuiInMemoryTable are now TypeScript!

Note

Had to fix babel PropTypes generator. It didn't generate appropriate checker for discriminated unions.
Wanted to use Omit but Eui uses 3.3.3. So, I commented the code out.

Checklist

Checked in dark mode
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

elasticmachine · 2019-10-15T07:53:20Z

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?

chandlerprall · 2019-10-15T15:04:12Z

Thank you very much for this effort! I'll start reviewing soon.

Wanted to use Omit but Eui uses 3.3.3. So, I commented the code out.

We have an Omit type defined in https://github.com/elastic/eui/blob/master/src/components/common.ts#L24

pugnascotia · 2019-10-16T08:35:25Z

src/components/basic_table/basic_table.tsx

+  sorting?: SortingType;
+}
+
+export type Props = CommonProps &


Could we export this as EuiBasicTableProps? I believe we've been moving in that direction recently.

Actually I renamed this to EuiBasicTableProps in my branch of your branch.

Then, I'll do that in that way.

Make it possible to type-check the props of EuiBasicTable and EuiInMemoryTable more effectively by making their props generic, and using the generic parameter throughout the tables' types.

pugnascotia · 2019-10-16T11:12:39Z

Thanks for the PR @sainthkh! I'll go through these changes shortly, but first I raised a PR against your branch:

sainthkh#1

These changes introduce a generic parameters to the basic table props, and threads that parameter through the types. The generic parameter makes it possible to get rid of some of the any types, and get better validation of e.g. prop names. My branch type-checks clean, but one test fails and I think it's because of your changes to the prop-types generator. Please run the following to see what I mean:

yarn test-unit src/components/selectable/selectable.test.tsx

pugnascotia · 2019-10-16T11:17:15Z

src/components/basic_table/__snapshots__/in_memory_table.test.tsx.snap

@@ -1007,7 +1004,7 @@ exports[`EuiInMemoryTable with pagination and selection 1`] = `
  responsive={true}
  selection={
    Object {
-      "onSelectionChanged": [Function],
+      "onSelectionChange": [Function],


Why has this been renamed? I think it should be done as a separate PR, assuming it's actually necessary.

onSelectionChanged was never correct, see #2432 / #2433

pugnascotia · 2019-10-16T11:45:19Z

src/components/basic_table/in_memory_table.tsx

-    items: [],
-    pagination: false,
-    sorting: false,
-    responsive: true,


We seem to have lost this prop completely. It supposed to be passed on to EuiBasicTable.

src/components/basic_table/loading_table_body.tsx

pugnascotia

The changes look good overall - @chandlerprall what do you think of also adding in generic support?

Co-Authored-By: Chandler Prall <chandler.prall@gmail.com>

…n't exist anymore.

chandlerprall · 2019-11-18T19:40:49Z

@sainthkh I'm done with my passes, I've got a handful of changes to add to your branch, if you would please update again with master I'll set up that PR. The i18ntokens.json conflict can be resolved by taking "their" side, any pending changes will be regenerated by the next build.

sainthkh · 2019-11-23T11:41:36Z

@chandlerprall Sorry, I misunderstood your comment and was waiting for your PR against my forked repo.

I've merged your branch first to my branch and merged again to the master branch.

chandlerprall · 2019-11-25T15:50:30Z

No worries! Everything looks good to me, and with the American Thanksgiving holiday this week I'd like to hold off merging this until next week, as we'll be less able to respond to any unintended consequences and it'd be more difficult to get Kibana upgraded.

Early next week I'll do another Kibana pass to make sure I have a clean branch there ready to go, and then we'll merge and release these changes.

Thank you, once again, for working through this conversion!

chandlerprall · 2019-12-06T15:30:44Z

Update: Took longer to update my Kibana branch against changes in master than expected, now planning to merging this on Monday the 9th.

sainthkh · 2019-12-07T01:03:18Z

@chandlerprall I agree. Kibana changes really fast. If there's anything to do on my side, please tell me.

chandlerprall

LGTM; lots of local digging & testing.

Thanks again @sainthkh for working on this and putting up with the back-and-forth communication & waiting!

sainthkh added 15 commits October 15, 2019 10:16

TypeScriptify custom_item_action.

ee2b251

TypeScriptify default_item_action.

250bd2b

Created action_types.ts.

ea8d657

Added missing props in DefaultItemAction.

df8a094

TypeScriptify expanded_item_actions.

766404b

TypeScriptify CollapsedItemActions.

a73bce9

TypeScriptify LoadingTableBody.

b1cbb97

TypeScriptify PaginationBar.

6e62a2c

TypeScriptify BasicTable.

c5b0e76

Removed anys.

781ed0e

Removed more anys.

dcf83da

Removed any in item and itemId.

95450c2

TypeScriptify EuiInMemoryTable.

f5565ed

Fixed bug in PropType generation.

ec27dd7

Clean up.

306c70e

chandlerprall self-requested a review October 15, 2019 15:02

chandlerprall mentioned this pull request Oct 15, 2019

Correct the tables' listing of onSelectionChanged -> onSelectionChange #2433

Merged

Used Omit.

06afcbf

pugnascotia reviewed Oct 16, 2019

View reviewed changes

Thread generics throughout EuiBasicTable

9ca68f6

Make it possible to type-check the props of EuiBasicTable and EuiInMemoryTable more effectively by making their props generic, and using the generic parameter throughout the tables' types.

pugnascotia reviewed Oct 16, 2019

View reviewed changes

pugnascotia added 3 commits October 16, 2019 12:20

Tweaks

d999c21

More tweaks

30fb9d2

Even more tweaks

049c959

pugnascotia reviewed Oct 16, 2019

View reviewed changes

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

pugnascotia reviewed Oct 16, 2019

View reviewed changes

chandlerprall and others added 15 commits November 18, 2019 09:09

Some typescript updates

cbd291e

ts->proptypes test for direct-usage literal values

548a89f

Update: State -> CollapsedItemActionsState

93ec991

Co-Authored-By: Chandler Prall <chandler.prall@gmail.com>

Update: State -> CollapsedItemActionsState

572716e

Co-Authored-By: Chandler Prall <chandler.prall@gmail.com>

Update State -> CustomItemActionState

b3820d6

Co-Authored-By: Chandler Prall <chandler.prall@gmail.com>

Update State -> CustomItemActionState

65dc1b1

Co-Authored-By: Chandler Prall <chandler.prall@gmail.com>

clean up types in in_memory_table

82e5e54

Fixed ExpandedItemActions snapshot. Because it has some props that do…

2de08e0

…n't exist anymore.

Remove more anys.

32c1e16

more types

75d9e32

guarantee the page argument in table callbacks if pagination is provided

18a39b4

rebased against master

10de16e

export EuiBasicTableColumn type

2b07266

Added TD attributes to field and computed column types

dda88bd

Accept string | number for EuiTableSortMobile's key

21d3836

sainthkh added 2 commits November 23, 2019 20:37

Merge chandlerprall/ts-basic-table into ts-basic-table.

d25f49c

Merge remote-tracking branch 'upstream/master' into ts-basic-table

d11b1c1

chandlerprall mentioned this pull request Nov 25, 2019

Typescriptify search source elastic/kibana#47644

Merged

chandlerprall mentioned this pull request Dec 6, 2019

Hide paginationBar in EuiBasicTable when there is no data #2598

Merged

2 tasks

Merge branch 'master' into saintkh-ts-basic-table

e3620a8

chandlerprall approved these changes Dec 9, 2019

View reviewed changes

changelog

8e513a3

chandlerprall merged commit 04662e8 into elastic:master Dec 9, 2019

chandlerprall mentioned this pull request Dec 10, 2019

Upgrade to EUI@17.1.2 [table typescript types] elastic/kibana#52688

Merged

snide added the typescript label Apr 17, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TypeScriptified basic_table. #2428

TypeScriptified basic_table. #2428

sainthkh commented Oct 15, 2019 •

edited

Loading

elasticmachine commented Oct 15, 2019

chandlerprall commented Oct 15, 2019 •

edited

Loading

pugnascotia Oct 16, 2019

pugnascotia Oct 16, 2019

sainthkh Oct 16, 2019

pugnascotia commented Oct 16, 2019

pugnascotia Oct 16, 2019

chandlerprall Oct 16, 2019

pugnascotia Oct 16, 2019

sainthkh Oct 16, 2019

pugnascotia left a comment

chandlerprall commented Nov 18, 2019 •

edited

Loading

sainthkh commented Nov 23, 2019

chandlerprall commented Nov 25, 2019

chandlerprall commented Dec 6, 2019 •

edited

Loading

sainthkh commented Dec 7, 2019

chandlerprall left a comment

TypeScriptified basic_table. #2428

TypeScriptified basic_table. #2428

Conversation

sainthkh commented Oct 15, 2019 • edited Loading

Summary

Note

Checklist

elasticmachine commented Oct 15, 2019

chandlerprall commented Oct 15, 2019 • edited Loading

pugnascotia Oct 16, 2019

Choose a reason for hiding this comment

pugnascotia Oct 16, 2019

Choose a reason for hiding this comment

sainthkh Oct 16, 2019

Choose a reason for hiding this comment

pugnascotia commented Oct 16, 2019

pugnascotia Oct 16, 2019

Choose a reason for hiding this comment

chandlerprall Oct 16, 2019

Choose a reason for hiding this comment

pugnascotia Oct 16, 2019

Choose a reason for hiding this comment

sainthkh Oct 16, 2019

Choose a reason for hiding this comment

pugnascotia left a comment

Choose a reason for hiding this comment

chandlerprall commented Nov 18, 2019 • edited Loading

sainthkh commented Nov 23, 2019

chandlerprall commented Nov 25, 2019

chandlerprall commented Dec 6, 2019 • edited Loading

sainthkh commented Dec 7, 2019

chandlerprall left a comment

Choose a reason for hiding this comment

sainthkh commented Oct 15, 2019 •

edited

Loading

chandlerprall commented Oct 15, 2019 •

edited

Loading

chandlerprall commented Nov 18, 2019 •

edited

Loading

chandlerprall commented Dec 6, 2019 •

edited

Loading