Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 5 commits into from

Conversation

jenny-s51
Copy link
Contributor

@jenny-s51 jenny-s51 commented Jul 19, 2019

What: in regards to #1950 , #2360 , and #2328. Snapshots are being generated for Table.test.tsx and tests are running successfully.

@jenny-s51 jenny-s51 requested a review from priley86 July 19, 2019 19:33
@priley86
Copy link
Member

hey @jenny-s51 ! Thanks a lot for this. I am reviewing it some more today. Should have more feedback for you soon!

@jenny-s51
Copy link
Contributor Author

@priley86 Looking forward to it -- thank you Patrick!

@priley86
Copy link
Member

Looking really good to me @jenny-s51! I've rebased these changes in #2360 and tests are now passing 😸

We will also need to rebase #2554 which just landed.

Still working on some other typescript conversions, but getting closer! One area of improvement here might be converting any of the any type references in the tests to types exposed in:

types.tsx (base reactabular table types):
https://github.com/patternfly/patternfly-react/blob/0b2a87b23023fc4e954ba25c1a82330f80ee2aac/packages/patternfly-4/react-table/src/components/Table/base/types.tsx

OR Table.tsx:

Types are in the base types.tsx are the base Reactabular types necessary for the existing table. My hope is that types exposed in Table.tsx are extended types created by us and exposed as "add-ons" to consumers (which inherit the base types where possible and add our additional API properties). We change this of course as we do API improvements, but this seems the simplest mental mapping at the moment.

Not urgent a.t.m. at all and big thanks for doing this!

@jenny-s51
Copy link
Contributor Author

@priley86 Thank you so much for all your help and feedback on this issue, Patrick! That sounds like a plan -- I'll put this in my to-do list and will plan to revisit the any type references as soon as I wrap up the bug-fixes I've been assigned for this sprint.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants