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

chore(table): fix types for table transforms #3203

Merged
merged 2 commits into from Nov 6, 2019

Conversation

@seanforyou23
Copy link
Member

seanforyou23 commented Oct 24, 2019

What: This PR updates the types used around the existing table decorators (transforms & formatters) so that consumers can specify the standard (ICell | string)[] type for table column definitions in TypeScript based apps. It also makes use of the existing types throughout a lot of the table related code, and modifies a few types slightly for consistency.

It should also help clear the strictFunctionTypes errors for Table.

Additional issues: #3148 and #3172 and #3224

@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Oct 24, 2019

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

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Oct 24, 2019

Codecov Report

Merging #3203 into master will increase coverage by 0.01%.
The diff coverage is 92%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3203      +/-   ##
==========================================
+ Coverage   67.43%   67.44%   +0.01%     
==========================================
  Files         892      892              
  Lines       24870    24866       -4     
  Branches     2141     2140       -1     
==========================================
  Hits        16771    16771              
+ Misses       7094     7091       -3     
+ Partials     1005     1004       -1
Flag Coverage Δ
#misc 95.45% <ø> (ø) ⬆️
#patternfly3 69.3% <ø> (ø) ⬆️
#patternfly4 64.78% <92%> (+0.02%) ⬆️
Impacted Files Coverage Δ
...-4/react-table/src/components/Table/base/types.tsx 100% <ø> (ø) ⬆️
...rnfly-4/react-table/src/components/Table/Table.tsx 91.25% <ø> (ø) ⬆️
...rc/components/Table/utils/decorators/headerCol.tsx 66.66% <0%> (+16.66%) ⬆️
...c/components/Table/utils/decorators/selectable.tsx 100% <100%> (ø) ⬆️
...src/components/Table/utils/decorators/sortable.tsx 100% <100%> (ø) ⬆️
...src/components/Table/utils/decorators/wrappable.ts 100% <100%> (ø) ⬆️
.../components/Table/utils/decorators/collapsible.tsx 100% <100%> (ø) ⬆️
.../components/Table/utils/decorators/cellActions.tsx 100% <100%> (ø) ⬆️
...src/components/Table/utils/decorators/cellWidth.ts 100% <100%> (ø) ⬆️
...-table/src/components/Table/utils/transformers.tsx 100% <100%> (ø) ⬆️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 29d89d9...e1b5c3b. Read the comment docs.

@seanforyou23

This comment has been minimized.

Copy link
Member Author

seanforyou23 commented Oct 24, 2019

Just realized we probably don't actually want to swap IExtra for IRowData in the places I've done currently in this PR, but I do think that's at least the right place to be making the adjustment. Flipping the strictFunctionTypes switch really brings the issues to light. Will keep hacking away!

@seanforyou23 seanforyou23 force-pushed the seanforyou23:transform-type-improvements branch from 28d0393 to 8ccd0c6 Oct 29, 2019
@seanforyou23 seanforyou23 requested a review from jenny-s51 Oct 29, 2019
@seanforyou23

This comment has been minimized.

Copy link
Member Author

seanforyou23 commented Oct 29, 2019

I've pushed some updates that improve the approach. It seems to be working well and I've only had to modify the tests slightly. I'm not sure why the children property type isn't inferred as something acceptable, will look into this more.

Beyond that, the main thing I'd like to get some opinion on is breaking out a shared return type for transformers and formatters. It seems like this may have been the original intention, but I'm partly guessing. Are transforms and formatters considered "subclass" of "decorators" or maybe "renderers"? I see references to both in the code and it isn't clear what the relationship is. The system will work in a variety of ways, just trying to match the correct terminology used to how the public api parts are named.

It seems to run fine in OpenShift on a quick test, and solves the type errors mentioned in the issues attached. I need to give it all another once over tomorrow and see if I can wrap these last bits up, would love any feedback in the meantime.

@seanforyou23 seanforyou23 force-pushed the seanforyou23:transform-type-improvements branch 2 times, most recently from ab789ce to 7c08adf Oct 30, 2019
@seanforyou23 seanforyou23 requested review from mturley and karelhala Oct 31, 2019
Copy link
Contributor

karelhala left a comment

This is great addition! We are not using types anywhere, but I know that folks will definetely be happy with new types checking! Just a question about one unused variable and we are good to go!

Copy link
Contributor

tlabaj left a comment

LGTM

…FunctionTypes tsconfig setting

apply types to demos for table
update tests
@seanforyou23 seanforyou23 force-pushed the seanforyou23:transform-type-improvements branch from 7c08adf to 63efb84 Nov 6, 2019
@seanforyou23 seanforyou23 force-pushed the seanforyou23:transform-type-improvements branch from 63efb84 to e1b5c3b Nov 6, 2019
@tlabaj
tlabaj approved these changes Nov 6, 2019
Copy link
Contributor

tlabaj left a comment

LGTM

@tlabaj tlabaj merged commit c0a63bc into patternfly:master Nov 6, 2019
8 checks passed
8 checks passed
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: build_integration Your tests passed on CircleCI!
Details
ci/circleci: build_pf3_docs Your tests passed on CircleCI!
Details
ci/circleci: build_pf4_docs Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: test_jest_other Your tests passed on CircleCI!
Details
ci/circleci: test_jest_pf4 Your tests passed on CircleCI!
Details
ci/circleci: upload_docs Your tests passed on CircleCI!
Details
@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Nov 6, 2019

Your changes have been released in:

  • @patternfly/react-docs@4.16.14
  • @patternfly/react-inline-edit-extension@2.12.20
  • demo-app-ts@3.9.9
  • @patternfly/react-integration@3.9.3
  • @patternfly/react-table@2.24.20
  • @patternfly/react-virtualized-extension@1.3.19

Thanks for your contribution! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.