Skip to content
This repository was archived by the owner on Dec 10, 2021. It is now read-only.

Conversation

@simcha90
Copy link
Contributor

@simcha90 simcha90 commented Mar 2, 2021

🏆 Enhancements
!Requires: https://github.com/apache/superset/pull/13417/files
This PR:

  1. add BE pagination to table with support of pages
  2. add setDataMask to buildQuery

📜 Documentation

Screen.Recording.2021-03-02.at.15.46.46.mov

@simcha90 simcha90 requested a review from a team as a code owner March 2, 2021 14:06
@vercel
Copy link

vercel bot commented Mar 2, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/superset/superset-ui/HwVs8e7qGyPHcTRTZTtXCxRSk7RF
✅ Preview: https://superset-ui-git-fork-simcha90-bepagination2-superset.vercel.app

@codecov
Copy link

codecov bot commented Mar 2, 2021

Codecov Report

Merging #985 (8d3fcd0) into master (0b48c32) will decrease coverage by 0.07%.
The diff coverage is 49.09%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #985      +/-   ##
==========================================
- Coverage   28.10%   28.03%   -0.08%     
==========================================
  Files         412      412              
  Lines        8411     8436      +25     
  Branches     1193     1199       +6     
==========================================
+ Hits         2364     2365       +1     
- Misses       5894     5917      +23     
- Partials      153      154       +1     
Impacted Files Coverage Δ
...art/registries/ChartBuildQueryRegistrySingleton.ts 100.00% <ø> (ø)
...-table/src/DataTable/components/SelectPageSize.tsx 40.00% <ø> (ø)
...in-chart-table/src/DataTable/utils/externalAPIs.ts 33.33% <0.00%> (-16.67%) ⬇️
plugins/plugin-chart-table/src/controlPanel.tsx 0.00% <ø> (-34.49%) ⬇️
plugins/plugin-chart-table/src/index.ts 0.00% <ø> (ø)
...ins/plugin-chart-table/src/DataTable/DataTable.tsx 38.82% <19.04%> (-6.39%) ⬇️
plugins/plugin-chart-table/src/buildQuery.ts 63.04% <58.82%> (+0.54%) ⬆️
plugins/plugin-chart-table/src/TableChart.tsx 55.55% <71.42%> (-0.92%) ⬇️
plugins/plugin-chart-table/src/transformProps.ts 64.47% <75.00%> (+1.65%) ⬆️
...es/superset-ui-core/src/query/buildQueryContext.ts 100.00% <100.00%> (ø)
... and 5 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 0b48c32...8d3fcd0. Read the comment docs.

Copy link
Contributor

@villebro villebro left a comment

Choose a reason for hiding this comment

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

First pass comments

Copy link
Contributor

@villebro villebro left a comment

Choose a reason for hiding this comment

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

This LGTM to me - I defer to @ktmud to sign off on this as he has most context with the table chart and requested some changes.

Copy link
Contributor

@ktmud ktmud left a comment

Choose a reason for hiding this comment

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

LGTM with one suggestion on reusing typing.

Comment on lines +4 to +15
export type BuildQuery<T = any> = (
formData: T,
options?: {
extras?: {
cachedChanges?: any;
};
hooks?: {
setDataMask: SetDataMaskHook;
setCachedChanges: (newChanges: any) => void;
};
},
) => QueryContext;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's possible to reuse BuildQueryFunction here:

Suggested change
export type BuildQuery<T = any> = (
formData: T,
options?: {
extras?: {
cachedChanges?: any;
};
hooks?: {
setDataMask: SetDataMaskHook;
setCachedChanges: (newChanges: any) => void;
};
},
) => QueryContext;
type BuildQuery = BuildQueryFunction<QueryFormData & JsonObject>;

You just provide a more relaxed FormData type.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants