feat(react-table): convert react-table to typescript by priley86 · Pull Request #2360 · patternfly/patternfly-react · GitHub

What:
Closes #1950 #2504 - React-Table TypeScript conversion.

Convert Table and many of the existing React-Table wrapping components to TSX
Add temporary .d.ts files for some components missing types so that react-table can be compiled w/ noImplicitAny set, cc: @ssilvert
update some faulty types in react-core (such as InternalDropdownItem)
convert base Reactabular source to TSX
reimplement base Reactabular usage of Context w/ modern React.createContext syntax
convert react-core Dropdown to typescript - we'll need DropdownPosition and DropdownDirection types in react-core at least
convert remaining components to typescript
fix jest tests/convert to typescript

Additional issues:

PatternFly-React preview: https://patternfly-react-pr-2360.surge.sh

cc: @jenny-s51 - please feel free to begin converting tests in react-table to typescript if you have time! I'm trying to tackle the remaining components now...

thanks for the help here!

cc: @mturley - will definitely try to review this w/ you once through w/ first pass!

Absolutely @priley86 ! At first glance this looks awesome, but I'll dig a little deeper and get back to you. Maybe early next week we can step through it together on a call.

This is probably outside the scope of this PR, but I notice you're pulling in tslint.. do you have any opinion on moving to eslint-typescript in the future? or at least enabling the prettier plugin for tslint? currently we're not enforcing code formatting in our CI: #2362

It might be nice to resolve that either before or shortly after we add a bunch of new code like this, so we don't have as much churn later on.

On 7/19/2019 3:13 PM, Mike Turley wrote: Absolutely @priley86 <https://github.com/priley86> ! At first glance this looks awesome, but I'll dig a little deeper and get back to you. Maybe early next week we can step through it together on a call. This is probably outside the scope of this PR, but I notice you're pulling in tslint.. do you have any opinion on moving to eslint-typescript in the future? or at least enabling the prettier plugin for tslint? currently we're not enforcing code formatting in our CI: #2362 <#2362> It might be nice to resolve that either before or shortly after we add a bunch of new code like this, so we don't have as much churn later on. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2360?email_source=notifications&email_token=AAD32QQZV2IDQPAWAQREBILQAIG5ZA5CNFSM4H3SPW6KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD2MQGRQ#issuecomment-513344326>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAD32QRVYBKHLGWSWR3FKULQAIG5ZANCNFSM4H3SPW6A>.

FWIW, Keycloak is using eslint with the @typescript-eslint/eslint-plugin and @typescript-eslint/parser. I chose this because the TypeScript team is embracing eslint over tslint: https://eslint.org/blog/2019/01/future-typescript-eslint

heh ;) - you guys read my mind, I am actually in full support of this move to @typescript-eslint/eslint-plugin and had brought this up in previous emails as well...

I chose to follow @patternfly/react-core, but I'm perfectly fine with making the shift off of tslint if others are. I don't know if this is something we want to start exploring here. This probably needs further discussion, but it's very important!

cc: @dgutride thoughts on this topic?

I realize there was some existing history and this move off of tslint will take some time...

@priley86 it sounds like it's outside the scope of this PR, but let's at least make sure we run Prettier on this code so it has a good start formatting-wise :) It might be worth getting Prettier to run as part of our tslint usage as a first step before we move to eslint.. I'm less concerned about which linter we have and more about just the fact that we have style rules disabled.

As for the rest of this PR, I'll be mostly buried in meetings the next two days, so I hope to start a more thorough review on Wednesday.

PatternFly-React preview: https://patternfly-react-pr-2360.surge.sh

@priley86 it sounds like it's outside the scope of this PR

Yea, i believe it should be a different PR, but I'm glad others support this move to @typescript-eslint/eslint-plugin too.

It might be worth getting Prettier to run as part of our tslint usage as a first step before we move to eslint

added tslint and some tslint fixes:
adf97b7

I've disabled these tslint rules since they will currently be breaking changes:

    "interface-name": false,
    "no-empty-interface": false,
    "ban-types": [true, {"Function": false}],

This is more or less just to conform to the existing standards in @patternfly/react-core

PatternFly-React preview: https://patternfly-react-pr-2360.surge.sh

(still probably out of scope, sorry to clutter this thread, but one last thing) I see you added tslint-config-prettier, but just fyi that brings us to the same problem we have in react-core. tslint-config-prettier disables tslint rules that would conflict with prettier, but it doesn't run prettier. we should also add tslint-plugin-prettier in both places (or set up Prettier CLI to be run as part of CI) #2362

PatternFly-React preview: https://patternfly-react-pr-2360.surge.sh

we should also add tslint-plugin-prettier in both places

agreed! It does not look like it's functioning correctly. I'll give this a go after other changes are complete!

PatternFly-React preview: https://patternfly-react-pr-2360.surge.sh

@dgutride I had approved (after building and installing with Cost management), but the current GitHub setting clears all approvals with each subsequent push.

rebased again and addressed above comment

Captured #2673 for followup...

Should be g2g!

Nice work! I pulled the simple example into the seed project and while it seems to render just fine, I get the following runtime warning;

Warning: Failed prop type: Invalid prop rows[1] supplied to Table.

FWIW I only see this when running in development mode, it isn't reported in production builds.

I don't see this with the other examples. The only other thing I noticed is that the "Table with headers that wrap" example also uses the CompactTable component name, I assume it's just from copy/pasta but could be confusing. Maybe give that example a more relevant name like WrappableHeadersTable or something of the sort?

@priley86 this was an insane amount of work, kudos! It will be really nice to have this all typesafe as a good foundation for evolving the table API.

I just have a few minor comments here and there (and some might be dumb questions, I'm still getting the hang of TypeScript). I apologize that it took me so long to find time to review this! I'll admit it was a little intimidating 😆

Oops, accidentally ticked the approve box on my last review.

Accidental approval

Nice work! I pulled the simple example into the seed project and while it seems to render just fine, I get the following runtime warning;

Warning: Failed prop type: Invalid prop rows[1] supplied to Table.

FWIW I only see this when running in development mode, it isn't reported in production builds.

I don't see this with the other examples. The only other thing I noticed is that the "Table with headers that wrap" example also uses the CompactTable component name, I assume it's just from copy/pasta but could be confusing. Maybe give that example a more relevant name like WrappableHeadersTable or something of the sort?

That warning is interesting (and technically correct). Our SimpleTable example is using the shorthand to pass the cells array without declaring that prop. This technically works b/c Reactabular will still merge those props, however I think we should make all examples consistent w/ our typings. This will probably also lead to more downstream consistency... so I've updated the example code to match typings to resolve this. Consumers can still use the shorthand, but I think the warning is appropriate...

Re: "WrappableHeadersTable" - updated!

Looks great! Awesome job.

@dlabrecq and @karelhala, you guys will need to re-approve, sorry for the churn!

thanks for the extensive review @mturley and @seanforyou23 ! Tried to capture all follow-ups in #2673. I believe all breaking changes related to types can come in that future PR... for now, just trying to keep existing consumers unaffected w/ this change.

👍

@dlabrecq @karelhala If you find anything wrong, please open a follow-up issue!

Your changes have been released in:

@patternfly/react-inline-edit-extension@2.10.0
@patternfly/react-table@2.17.0

Thanks for your contribution! 🎉

priley86 added Do Not Merge PF4 labels Jun 26, 2019

priley86 mentioned this pull request Jun 26, 2019

Convert React-Table to TypeScript #1950

Closed

priley86 force-pushed the react-table-typescript branch from 9d7b156 to e6197e3 Compare June 26, 2019 14:21

tlabaj requested a review from karelhala June 27, 2019 18:20

tlabaj assigned dgutride Jun 27, 2019

tlabaj requested a review from dgutride June 27, 2019 18:20

priley86 force-pushed the react-table-typescript branch from ee460d4 to c4e2d29 Compare July 19, 2019 12:08

jenny-s51 mentioned this pull request Jul 19, 2019

fix(jest tests): converted table.test.js to TSX #2553

Closed

redallen mentioned this pull request Jul 22, 2019

fix(react-table): Add exports for compoundExpand. #2554

Merged

priley86 force-pushed the react-table-typescript branch from adf97b7 to 84a943d Compare July 22, 2019 15:17

priley86 force-pushed the react-table-typescript branch from 2ce7b4b to 72be8ce Compare July 22, 2019 16:28

priley86 force-pushed the react-table-typescript branch from 72be8ce to 5eb9fb7 Compare July 22, 2019 17:38

dlabrecq previously approved these changes Aug 8, 2019

View reviewed changes

priley86 mentioned this pull request Aug 8, 2019

First pass at updating the examples for the patternfly/react-table library #2672

Closed

priley86 added 3 commits August 8, 2019 15:12

feat(react-table): convert react-table to typescript

28ef6de

remove IDecorator for now

df31290

add rest Context props to Body

fe06c8d

priley86 dismissed stale reviews from dlabrecq, karelhala, and dgutride via fe06c8d August 8, 2019 19:12

priley86 force-pushed the react-table-typescript branch from 61ff3bc to fe06c8d Compare August 8, 2019 19:12

priley86 mentioned this pull request Aug 8, 2019

PF4 React-Table - TypeScript followup #2673

Closed

7 tasks

karelhala previously approved these changes Aug 9, 2019

View reviewed changes

seanforyou23 reviewed Aug 9, 2019

View reviewed changes

mturley previously approved these changes Aug 9, 2019

View reviewed changes

mturley reviewed Aug 9, 2019

View reviewed changes

review updates

05feabf

priley86 dismissed karelhala’s stale review via 05feabf August 9, 2019 19:05

priley86 mentioned this pull request Aug 9, 2019

PF4 - React Table - Inline Edit Feature #2679

Closed

mturley approved these changes Aug 9, 2019

View reviewed changes

seanforyou23 approved these changes Aug 9, 2019

View reviewed changes

priley86 mentioned this pull request Aug 12, 2019

feat(Table): add new visibleOn and hiddenOn breakpoint #2676

Merged

mturley requested review from dlabrecq and karelhala August 13, 2019 18:47

redallen merged commit d0ab9c0 into patternfly:master Aug 14, 2019

feat(react-table): convert react-table to typescript by priley86 · Pull Request #2360 · patternfly/patternfly-react · GitHub

Uh oh!

feat(react-table): convert react-table to typescript - #2360

feat(react-table): convert react-table to typescript#2360redallen merged 4 commits intopatternfly:masterpatternfly/patternfly-react:masterfrom priley86:react-table-typescriptpriley86/patternfly-react:react-table-typescriptCopy head branch name to clipboard

Conversation

priley86 commented Jun 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

patternfly-build commented Jun 26, 2019

Uh oh!

priley86 commented Jul 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

priley86 commented Jul 19, 2019

Uh oh!

mturley commented Jul 19, 2019

Uh oh!

ssilvert commented Jul 19, 2019 via email

Uh oh!

priley86 commented Jul 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

priley86 commented Jul 19, 2019

Uh oh!

mturley commented Jul 22, 2019

Uh oh!

patternfly-build commented Jul 22, 2019

Uh oh!

priley86 commented Jul 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

patternfly-build commented Jul 22, 2019

Uh oh!

patternfly-build commented Jul 22, 2019

Uh oh!

mturley commented Jul 22, 2019

Uh oh!

patternfly-build commented Jul 22, 2019

Uh oh!

priley86 commented Jul 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

patternfly-build commented Jul 22, 2019

Uh oh!

dlabrecq left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

priley86 commented Aug 8, 2019

Uh oh!

seanforyou23 left a comment

Choose a reason for hiding this comment

Uh oh!

mturley left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mturley left a comment

Choose a reason for hiding this comment

Uh oh!

priley86 commented Aug 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mturley left a comment

Choose a reason for hiding this comment

Uh oh!

mturley commented Aug 9, 2019

Uh oh!

priley86 commented Aug 9, 2019

Uh oh!

seanforyou23 left a comment

feat(react-table): convert react-table to typescript#2360
redallen merged 4 commits into
patternfly:masterfrom
priley86:react-table-typescript

priley86 commented Jun 26, 2019 •
edited
Loading

priley86 commented Jul 19, 2019 •
edited
Loading

priley86 commented Jul 19, 2019 •
edited
Loading

priley86 commented Jul 22, 2019 •
edited
Loading

priley86 commented Jul 22, 2019 •
edited
Loading

dlabrecq left a comment •
edited
Loading

priley86 commented Aug 9, 2019 •
edited
Loading