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: Update typing for Table component #22832

Closed
wants to merge 4 commits into from

Conversation

codyml
Copy link
Member

@codyml codyml commented Jan 23, 2023

SUMMARY

This PR resolves some typing and props issues in the Table component:

  • Removes some unused type/interface definitions and type annotations
  • Imports some types directly from AntD rather than redefining them
  • Removes uses of any
  • Makes the Table component type and its corresponding TableProps interface generic, requiring the RecordType type variable. This makes Table more compatible with the AntD components/types it extends.
  • Stops TableProps from inheriting from its AntD counterpart. Our Table component only knows how to handle a subset of the properties in the AntD TableProps, but it currently accepts every prop defined in AntD TableProps.
  • Fixes support for the onRow prop, which is described in Storybook but isn't currently working.
  • Updates imports so we're only importing things that are exposed in antd/lib/table, antd/lib/config-provider, and antd/lib/pagination
  • Fixes the jsdoc for the selectionType prop

TESTING INSTRUCTIONS

  • There should be no user-facing changes
  • Check that Storybook "Actions" tab now registers actions

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@codecov
Copy link

codecov bot commented Jan 23, 2023

Codecov Report

Merging #22832 (a31afe2) into master (5d550f4) will not change coverage.
The diff coverage is 75.00%.

❗ Current head a31afe2 differs from pull request most recent head 165f132. Consider uploading reports for the commit 165f132 to get more accurate results

@@           Coverage Diff           @@
##           master   #22832   +/-   ##
=======================================
  Coverage   67.42%   67.42%           
=======================================
  Files        1877     1877           
  Lines       72110    72110           
  Branches     7863     7863           
=======================================
  Hits        48621    48621           
  Misses      21470    21470           
  Partials     2019     2019           
Flag Coverage Δ
javascript 53.80% <75.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...c/components/Chart/DrillDetail/DrillDetailPane.tsx 75.00% <ø> (ø)
superset-frontend/src/components/Table/index.tsx 70.23% <60.00%> (ø)
...set-frontend/src/components/Table/VirtualTable.tsx 73.43% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@codyml codyml changed the title [WIP] chore: Update typing for Table component chore: Update typing for Table component Jan 23, 2023
@@ -302,7 +298,7 @@ export default function DrillDetailPane({
recordCount={resultsPage?.total}
usePagination
loading={isLoading}
onChange={(pagination: TablePaginationConfig) =>
Copy link
Member

Choose a reason for hiding this comment

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

wouldn't we want to leave the annotations to make it more clear on what data is being passed from the function?

Copy link
Member Author

Choose a reason for hiding this comment

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

I took it out because TS can infer this type, so you'll still see TablePaginationConfig as the type if you inspect in your IDE and you'll still get the same lint errors as if it were explicitly annotated, and I usually wouldn't include an annotation in those cases to keep the code cleaner. But, no strong feelings – do you think explicit annotation is helpful here?

@rusackas rusackas requested a review from etr2460 January 26, 2023 07:21
@codyml codyml force-pushed the chore/table-misc branch 2 times, most recently from ab45c95 to fcd364d Compare February 1, 2023 02:29
@codyml
Copy link
Member Author

codyml commented Feb 3, 2023

/testenv up

@github-actions
Copy link
Contributor

github-actions bot commented Feb 3, 2023

@codyml Ephemeral environment creation is currently limited to committers.

@codyml
Copy link
Member Author

codyml commented Feb 3, 2023

/testenv up

@github-actions
Copy link
Contributor

github-actions bot commented Feb 3, 2023

@codyml Ephemeral environment spinning up at http://52.32.70.238:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

@codyml codyml closed this Feb 9, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Feb 9, 2023

Ephemeral environment shutdown and build artifacts deleted.

@eschutho eschutho reopened this Feb 13, 2023
@codyml codyml closed this May 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants