FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

chore(table): fix types for table transforms by seanforyou23 · Pull Request #3203 · patternfly/patternfly-react · GitHub

/ patternfly-react Public

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

Merged
tlabaj merged 2 commits into
patternfly:masterfrom
seanforyou23:transform-type-improvements
Nov 6, 2019
ConversationCommits (2)ChecksFiles changed
Merged

chore(table): fix types for table transforms#3203
tlabaj merged 2 commits into
patternfly:masterfrom
seanforyou23:transform-type-improvements

Conversation

seanforyou23 commented Oct 24, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

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 commented Oct 24, 2019

Copy link
Copy Markdown
Collaborator

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

codecov-io commented Oct 24, 2019 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

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

@@            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 commented Oct 24, 2019

Copy link
Copy Markdown
Contributor Author

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 force-pushed the transform-type-improvements branch from 28d0393 to 8ccd0c6 Compare October 29, 2019 20:31

seanforyou23 commented Oct 29, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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 force-pushed the transform-type-improvements branch 2 times, most recently from ab789ce to 7c08adf Compare October 30, 2019 21:13
karelhala reviewed Nov 5, 2019
View reviewed changes

karelhala left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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!

Comment thread packages/patternfly-4/react-table/src/components/Table/utils/decorators/sortable.tsx Outdated
tlabaj previously approved these changes Nov 5, 2019
View reviewed changes

tlabaj left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

fix(table): fix table types so transforms/formatters work with strict…
0cb7dce 
…FunctionTypes tsconfig setting

apply types to demos for table
update tests
seanforyou23 force-pushed the transform-type-improvements branch from 7c08adf to 63efb84 Compare November 6, 2019 15:52
seanforyou23 force-pushed the transform-type-improvements branch from 63efb84 to e1b5c3b Compare November 6, 2019 15:59
tlabaj approved these changes Nov 6, 2019
View reviewed changes

tlabaj left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

tlabaj merged commit c0a63bc into patternfly:master Nov 6, 2019

patternfly-build commented Nov 6, 2019

Copy link
Copy Markdown
Collaborator

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! 🎉

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

dlabaj Awaiting requested review from dlabaj
jeff-phillips-18 Awaiting requested review from jeff-phillips-18
TheRealJon Awaiting requested review from TheRealJon
jenny-s51 Awaiting requested review from jenny-s51
mturley Awaiting requested review from mturley
3 more reviewers
karelhala karelhala left review comments
dlabrecq dlabrecq approved these changes
tlabaj tlabaj approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

kmcfaul

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

Back | FazBrowse Home | New Git URL