feat(table): enable table filter and better typing by ktmud · Pull Request #344 · apache-superset/superset-ui

ktmud · 2020-04-06T23:48:31Z

🏆 Enhancements
🐛 Bug Fix

Add back "emit filter" support to Table chart.
Refactor typing for both Table and Big Number, make more strict or give more hints when possible.

vercel · 2020-04-06T23:48:35Z

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

🔍 Inspect: https://zeit.co/superset/superset-ui/fq9g998vh
✅ Preview: https://superset-ui-git-fork-ktmud-fix-clientside-filter.superset.now.sh

packages/superset-ui-chart/src/models/ChartProps.ts

codecov · 2020-04-06T23:55:24Z

Codecov Report

Merging #344 into master will decrease coverage by 0.09%.
The diff coverage is 40.67%.

@@            Coverage Diff             @@
##           master     #344      +/-   ##
==========================================
- Coverage   26.35%   26.25%   -0.10%     
==========================================
  Files         192      192              
  Lines        5256     5287      +31     
  Branches      459      473      +14     
==========================================
+ Hits         1385     1388       +3     
- Misses       3842     3864      +22     
- Partials       29       35       +6

Impacted Files	Coverage Δ
...rset-ui-chart/src/components/ChartDataProvider.tsx	`100.00% <ø> (ø)`
...number/src/utils/getTimeFormatterForGranularity.ts	`83.33% <0.00%> (ø)`
plugins/big-number/src/BigNumber/transformProps.ts	`52.00% <11.11%> (-5.15%)`	⬇️
plugins/table/src/ReactDataTable.tsx	`64.86% <42.10%> (-14.90%)`	⬇️
plugins/table/test/testData.ts	`83.33% <50.00%> (-16.67%)`	⬇️
plugins/table/src/transformProps.ts	`48.14% <57.14%> (-3.86%)`	⬇️
...kages/superset-ui-chart/src/clients/ChartClient.ts	`100.00% <100.00%> (ø)`
...ackages/superset-ui-chart/src/models/ChartProps.ts	`100.00% <100.00%> (ø)`

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 fb9cce3...231bea3. Read the comment docs.

…344) Bumps [@airbnb/config-typescript](https://github.com/airbnb/nimbus) from 2.2.2 to 2.2.3. - [Release notes](https://github.com/airbnb/nimbus/releases) - [Changelog](https://github.com/airbnb/nimbus/blob/master/CHANGELOG.md) - [Commits](https://github.com/airbnb/nimbus/compare/@airbnb/config-typescript@2.2.2...@airbnb/config-typescript@2.2.3) Signed-off-by: dependabot-preview[bot] <support@dependabot.com>

packages/superset-ui-chart-composition/package.json

packages/superset-ui-chart/src/models/ChartProps.ts

plugins/big-number/src/BigNumber/transformProps.ts

kristw · 2020-04-07T22:28:52Z

plugins/big-number/src/BigNumber/transformProps.ts

+  compareLag?: string | number;
+  yAxisFormat?: string;
+  timeGrainSqla?: TimeGranularity;
+} & ChartProps['formData'];


Does it need & ChartProps['formData']?

Thought it was more semantically correct to include it. ~~Can probably drop since we've been exhaustive listing all the fields.~~ Actually, it'd be easier to just do this because some shared properties are indeed useful to keep in ChartProps and it doesn't make sense to exhaustively list them for every chart type.

two questions:

should it be Partial<ChartProps['formData']>? or all of them?

are there duplicate keys in ChartProps['formData']? I'm not sure order of override for TS, just wanted to make sure if there are duplicates that the BigNumberFormData values take precedent.

~~Actually, it'd be easier to just do this because some shared properties are indeed useful to keep in ChartProps and it doesn't make sense to exhaustively list them for every chart type.~~

Strike again. formData and queryData are basically PlainObject, which accepts any key with any values. Partial or not it doesn't make any difference, at least for now. I was thinking & ChartProps.

@williaster thanks for the reminder about overrides. I should have placed ChartProps['formData'] at the front.

I think I'm going to avoid Partial as it makes all properties optional, which is probably not what we want in this case.

williaster

lgtm overall 👍

packages/superset-ui-chart/src/clients/ChartClient.ts

packages/superset-ui-chart/src/models/ChartProps.ts

williaster · 2020-04-20T18:27:37Z

packages/superset-ui-query/src/types/Datasource.ts

  metrics: QueryObjectMetric[];
+  description?: string;
+  columnFormats?: {
+    [key: string]: string;


could we be any more specific about what key is here + verboseMap below? like name or id or something?

I always thought it's good to be generic in indexable types so that new TypeScript users don't confuse it with an actual property name.

I'll add some inline comments instead.

williaster · 2020-04-20T18:30:04Z

plugins/big-number/src/BigNumber/transformProps.ts

+  compareLag?: string | number;
+  yAxisFormat?: string;
+  timeGrainSqla?: TimeGranularity;
+} & ChartProps['formData'];


two questions:

should it be Partial<ChartProps['formData']>? or all of them?

are there duplicate keys in ChartProps['formData']? I'm not sure order of override for TS, just wanted to make sure if there are duplicates that the BigNumberFormData values take precedent.

williaster · 2020-04-20T18:31:08Z

plugins/big-number/src/BigNumber/transformProps.ts

  const {
    colorPicker,
-    compareLag: compareLagInput,
+    compareLag: compareLag_,


readability nit – I think compareLagInput is a clearer name, not sure what _ means tho I guess it's used a few places here

I use dangling "_" for arguments that will be immediately overridden by simple argument processing. It indicates a sense of temporariness and keeps the variable names clean.

williaster · 2020-04-20T18:33:04Z

plugins/table/src/ReactDataTable.tsx

        value = new Date(value);
      }
-      return formatTimestamp(value) as string;
+      return formatTimestamp(value as Date | number | null) as string;


can you set this above instead of coercing? let value: Date | number | null = val; ...

Actually tried that, but ran into error when passing it to new Date. Obviously new Date doesn't accept nulls.

williaster · 2020-04-20T18:35:42Z

plugins/table/src/Table.css

+  cursor: pointer;
+}
+.superset-legacy-chart-table td.dt-is-filter:hover {
+  background-color: linen;


curious how these were chosen? should we get any design feedback?

The original color is:

background-color: lighten(desaturate(@brand-primary, 50%), 50%);

apache/superset@cbf38ff#diff-9479060a8cb28b444d0ce7f5df0a33ccL577

Which translates to #bce9e5. I just picked a named color that is close to it and does not look bad. Wanted to use the original color, but it kind of looked too dark to me.

This will probably eventually become customizable with the introduction of the superset-ui/style package.

ktmud requested a review from a team as a code owner April 6, 2020 23:48

vercel bot deployed to Preview April 6, 2020 23:48 View deployment

pull-request-size bot added the size/L label Apr 6, 2020

ktmud commented Apr 6, 2020

View reviewed changes

packages/superset-ui-chart/src/models/ChartProps.ts Outdated Show resolved Hide resolved

ktmud force-pushed the fix-clientside-filter branch from 906912a to 79fdd3c Compare April 6, 2020 23:54

vercel bot deployed to Preview April 6, 2020 23:54 View deployment

ktmud force-pushed the fix-clientside-filter branch from 79fdd3c to d80da80 Compare April 16, 2020 23:17

vercel bot deployed to Preview April 16, 2020 23:17 View deployment

kristw reviewed Apr 17, 2020

View reviewed changes

ktmud force-pushed the fix-clientside-filter branch from d80da80 to c33963f Compare April 17, 2020 20:18

vercel bot deployed to Preview April 17, 2020 20:18 View deployment

ktmud force-pushed the fix-clientside-filter branch from c33963f to becf76d Compare April 20, 2020 06:50

vercel bot requested a deployment to Preview April 20, 2020 06:50 Abandoned

ktmud force-pushed the fix-clientside-filter branch from becf76d to dac787a Compare April 20, 2020 07:01

vercel bot requested a deployment to Preview April 20, 2020 07:01 Abandoned

ktmud mentioned this pull request Apr 20, 2020

fix: re-enable table filter apache/superset#9593

Closed

3 tasks

ktmud force-pushed the fix-clientside-filter branch from dac787a to 0c2d5e3 Compare April 20, 2020 17:36

vercel bot had a problem deploying to Preview April 20, 2020 17:36 Failure

apache-superset deleted a comment from vercel bot Apr 20, 2020

feat: enable table filter and better typing

87f3440

ktmud force-pushed the fix-clientside-filter branch from 0c2d5e3 to 87f3440 Compare April 20, 2020 17:48

vercel bot deployed to Preview April 20, 2020 17:48 View deployment

williaster approved these changes Apr 20, 2020

View reviewed changes

Refactor how ChartProps get overrides

95c3a36

vercel bot deployed to Preview April 21, 2020 00:02 View deployment

ktmud changed the title ~~feat: enable table filter and better typing~~ feat(table): enable table filter and better typing Apr 21, 2020

Update doc strings

231bea3

vercel bot deployed to Preview April 21, 2020 00:36 View deployment

ktmud merged commit fdf6514 into apache-superset:master Apr 21, 2020

ktmud deleted the fix-clientside-filter branch April 21, 2020 01:03

Conversation

ktmud commented Apr 6, 2020

Uh oh!

vercel bot commented Apr 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Apr 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ktmud Apr 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

williaster left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ktmud Apr 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ktmud Apr 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

vercel bot commented Apr 6, 2020 •

edited

Loading

codecov bot commented Apr 6, 2020 •

edited

Loading

ktmud Apr 17, 2020 •

edited

Loading

ktmud Apr 20, 2020 •

edited

Loading

ktmud Apr 20, 2020 •

edited

Loading