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

feat(table): enable table filter and better typing #344

Merged
merged 3 commits into from
Apr 21, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@
"@types/enzyme": "^3.10.3",
"@types/jest": "^25.1.1",
"@types/jsdom": "^12.2.4",
"@types/react-test-renderer": "^16.9.0",
"@types/react-test-renderer": "^16.9.2",
"enzyme": "^3.10.0",
"enzyme-adapter-react-16": "^1.15.1",
"enzyme-to-json": "^3.4.3",
Expand All @@ -73,9 +73,9 @@
"jest-mock-console": "^1.0.0",
"lerna": "^3.15.0",
"lint-staged": "^10.0.3",
"react": "^16.9.0",
"react-dom": "^16.9.0",
"react-test-renderer": "^16.9.0"
"react": "^16.13.1",
"react-dom": "^16.13.1",
"react-test-renderer": "^16.13.1"
},
"engines": {
"node": ">=10.10.0",
Expand Down
4 changes: 2 additions & 2 deletions packages/superset-ui-chart-composition/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,12 @@
"access": "public"
},
"dependencies": {
"@types/react": "^16.7.17",
"@types/react": "^16.9.34",
"@vx/responsive": "^0.0.195",
"csstype": "^2.6.4"
},
"peerDependencies": {
"@superset-ui/core": "^0.12.0",
"react": "^16.7.17"
"react": "^16.13.1"
ktmud marked this conversation as resolved.
Show resolved Hide resolved
}
}
4 changes: 2 additions & 2 deletions packages/superset-ui-chart/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
"access": "public"
},
"dependencies": {
"@types/react": "^16.7.17",
"@types/react": "^16.9.34",
"@types/react-loadable": "^5.4.2",
"@vx/responsive": "^0.0.195",
"prop-types": "^15.6.2",
Expand All @@ -45,6 +45,6 @@
"@superset-ui/core": "^0.12.0",
"@superset-ui/dimension": "^0.12.0",
"@superset-ui/query": "^0.12.0",
"react": "^16.7.17"
"react": "^16.13.1"
}
}
7 changes: 5 additions & 2 deletions packages/superset-ui-chart/src/clients/ChartClient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import {
import { QueryFormData, Datasource } from '@superset-ui/query';
import getChartBuildQueryRegistry from '../registries/ChartBuildQueryRegistrySingleton';
import getChartMetadataRegistry from '../registries/ChartMetadataRegistrySingleton';
import { QueryData } from '../models/ChartProps';
import { QueryData } from '../types/QueryResponse';
import { AnnotationLayerMetadata } from '../types/Annotation';
import { PlainObject } from '../types/Base';

Expand Down Expand Up @@ -94,7 +94,10 @@ export default class ChartClient {
},
...options,
} as RequestConfig)
.then(response => response.json as Json);
.then(response => {
// let's assume response.json always has the shape of QueryData
return response.json as QueryData;
ktmud marked this conversation as resolved.
Show resolved Hide resolved
});
}

return Promise.reject(new Error(`Unknown chart type: ${visType}`));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import React, { ReactNode } from 'react';
import { SupersetClientInterface, RequestConfig } from '@superset-ui/connection';
import { QueryFormData, Datasource } from '@superset-ui/query';
import ChartClient, { SliceIdAndOrFormData } from '../clients/ChartClient';
import { QueryData } from '../models/ChartProps';
import { QueryData } from '../types/QueryResponse';

interface Payload {
formData: Partial<QueryFormData>;
Expand Down
1 change: 1 addition & 0 deletions packages/superset-ui-chart/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,3 +16,4 @@ export { default as getChartTransformPropsRegistry } from './registries/ChartTra
export { default as ChartDataProvider } from './components/ChartDataProvider';

export * from './types/TransformFunction';
export * from './types/QueryResponse';
36 changes: 18 additions & 18 deletions packages/superset-ui-chart/src/models/ChartProps.ts
Original file line number Diff line number Diff line change
@@ -1,30 +1,32 @@
import { createSelector } from 'reselect';
import { convertKeysToCamelCase } from '@superset-ui/core';
import { Datasource } from '@superset-ui/query';
import { HandlerFunction, PlainObject } from '../types/Base';
import { QueryData, DataRecordFilters } from '../types/QueryResponse';

// TODO: more specific typing for these fields of ChartProps
type AnnotationData = PlainObject;
type CamelCaseDatasource = PlainObject;
type SnakeCaseDatasource = PlainObject;
type CamelCaseFormData = PlainObject;
type SnakeCaseFormData = PlainObject;
export type QueryData = PlainObject;
/** Initial values for the visualizations, currently used by only filter_box and table */
type InitialValues = PlainObject;
type RawFormData = CamelCaseFormData | SnakeCaseFormData;

type ChartPropsSelector = (c: ChartPropsConfig) => ChartProps;

/** Optional field for event handlers, renderers */
type Hooks = {
/** handle adding filters */
onAddFilter?: HandlerFunction;
/** handle errors */
/**
* sync active filters between chart and dashboard, "add" actually
* also handles "change" and "remove".
*/
onAddFilter?: (newFilters: DataRecordFilters, merge?: boolean) => void;
ktmud marked this conversation as resolved.
Show resolved Hide resolved
/** handle errors */
onError?: HandlerFunction;
/** use the vis as control to update state */
setControlValue?: HandlerFunction;
/** handle tooltip */
setTooltip?: HandlerFunction;
[key: string]: any;
};
} & PlainObject;
ktmud marked this conversation as resolved.
Show resolved Hide resolved

/**
* Preferred format for ChartProps config
Expand All @@ -37,9 +39,9 @@ export interface ChartPropsConfig {
* Formerly called "filters", which was misleading because it is actually
* initial values of the filter_box and table vis
*/
initialValues?: InitialValues;
initialValues?: DataRecordFilters;
/** Main configuration of the chart */
formData?: SnakeCaseFormData;
formData?: RawFormData;
/** Chart height */
height?: number;
/** Programmatic overrides such as event handlers, renderers */
Expand All @@ -53,22 +55,20 @@ export interface ChartPropsConfig {
const DEFAULT_WIDTH = 800;
const DEFAULT_HEIGHT = 600;

export default class ChartProps<
FormDataType extends CamelCaseFormData | SnakeCaseFormData = CamelCaseFormData
> {
export default class ChartProps {
static createSelector: () => ChartPropsSelector;

annotationData: AnnotationData;

datasource: CamelCaseDatasource;
datasource: Datasource;

rawDatasource: SnakeCaseDatasource;

initialValues: InitialValues;
initialValues: DataRecordFilters;

formData: CamelCaseFormData;

rawFormData: SnakeCaseFormData | CamelCaseFormData;
rawFormData: RawFormData;

height: number;

Expand All @@ -82,7 +82,7 @@ export default class ChartProps<
const {
annotationData = {},
datasource = {},
formData = {} as FormDataType,
formData = {},
ktmud marked this conversation as resolved.
Show resolved Hide resolved
hooks = {},
initialValues = {},
queryData = {},
Expand Down
18 changes: 18 additions & 0 deletions packages/superset-ui-chart/src/types/QueryResponse.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
/**
* Types for query response
*/
import { PlainObject } from './Base';

export type DataRecordValue = number | string | boolean | Date | null;

export interface DataRecord {
[key: string]: DataRecordValue;
}

// data record value filters from FilterBox
export interface DataRecordFilters {
[key: string]: DataRecordValue[];
}

// the response json from query API
export type QueryData = PlainObject;
6 changes: 3 additions & 3 deletions packages/superset-ui-chart/test/models/ChartProps.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,10 @@ const RAW_FORM_DATA = {
};

const RAW_DATASOURCE = {
another_field: 2,
column_formats: { test: '%s' },
};

const QUERY_DATA = {};
const QUERY_DATA = { data: {} };

describe('ChartProps', () => {
it('exists', () => {
Expand All @@ -33,7 +33,7 @@ describe('ChartProps', () => {
queryData: QUERY_DATA,
});
expect(props.formData.someField).toEqual(1);
expect(props.datasource.anotherField).toEqual(2);
expect(props.datasource.columnFormats).toEqual(RAW_DATASOURCE.column_formats);
expect(props.rawFormData).toEqual(RAW_FORM_DATA);
expect(props.rawDatasource).toEqual(RAW_DATASOURCE);
});
Expand Down
2 changes: 1 addition & 1 deletion packages/superset-ui-demo/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@
"core-js": "3.6.5",
"gh-pages": "^2.2.0",
"jquery": "^3.4.1",
"react": "^16.6.0",
"react": "^16.13.1",
"storybook-addon-jsx": "^7.1.0"
},
"devDependencies": {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,7 @@
"fetch_values_predicate": null,
"template_params": null
},
"initialValues": {},
"activeFilters": {},
"formData": {
"datasource": "3__table",
"viz_type": "table",
Expand Down Expand Up @@ -300,7 +300,7 @@
"table_timestamp_format": "%Y-%m-%d %H:%M:%S",
"page_length": 0,
"include_search": true,
"table_filter": false,
"table_filter": true,
"align_pn": false,
"color_pn": true
},
Expand Down Expand Up @@ -419,7 +419,7 @@
"table_timestamp_format": "%Y-%m-%d %H:%M:%S",
"page_length": 0,
"include_search": true,
"table_filter": false,
"table_filter": true,
"align_pn": false,
"color_pn": true,
"where": "",
Expand Down
9 changes: 8 additions & 1 deletion packages/superset-ui-query/src/types/Datasource.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,15 @@ export enum DatasourceType {
export interface Datasource {
id: number;
name: string;
description?: string;
type: DatasourceType;
columns: Column[];
metrics: QueryObjectMetric[];
description?: string;
// key is column names (labels)
columnFormats?: {
[key: string]: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add some inline comments instead.

};
verboseMap?: {
[key: string]: string;
};
}
2 changes: 2 additions & 0 deletions packages/superset-ui-query/src/types/Query.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ export type QueryObjectFilterClause = {

export type QueryObjectMetric = {
label: string;
metric_name?: string;
d3format?: string;
} & Partial<AdhocMetric>;

export type QueryObjectExtras = Partial<{
Expand Down
67 changes: 48 additions & 19 deletions plugins/big-number/src/BigNumber/transformProps.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,25 +19,56 @@
import * as color from 'd3-color';
import { getNumberFormatter, NumberFormats } from '@superset-ui/number-format';
import { ChartProps } from '@superset-ui/chart';
import getTimeFormatterForGranularity from '../utils/getTimeFormatterForGranularity';
import getTimeFormatterForGranularity, {
TimeGranularity,
} from '../utils/getTimeFormatterForGranularity';

const TIME_COLUMN = '__timestamp';
const formatPercentChange = getNumberFormatter(NumberFormats.PERCENT_SIGNED_1_POINT);

export interface DatasourceMetric {
label: string;
// eslint-disable-next-line camelcase
metric_name?: string;
d3format?: string;
}

// we trust both the x (time) and y (big number) to be numeric
type BigNumberDatum = {
[TIME_COLUMN]: number;
export interface BigNumberDatum {
[key: string]: number | null;
}

export type BigNumberFormData = {
colorPicker?: {
r: number;
g: number;
b: number;
};
metric?:
| {
label: string;
}
| string;
compareLag?: string | number;
yAxisFormat?: string;
timeGrainSqla?: TimeGranularity;
};

export default function transformProps(chartProps: ChartProps) {
const { width, height, formData, queryData } = chartProps;
export type BignumberChartProps = ChartProps & {
formData: BigNumberFormData;
queryData: ChartProps['queryData'] & {
data?: BigNumberDatum[];
};
};

export default function transformProps(chartProps: BignumberChartProps) {
const { width, height, queryData, formData } = chartProps;
const {
colorPicker,
compareLag: compareLagInput,
compareLag: compareLag_,
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@ktmud ktmud Apr 20, 2020

Choose a reason for hiding this comment

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

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.

compareSuffix = '',
headerFontSize,
metric,
metric = 'value',
showTrendLine,
startYAxisAtZero,
subheader = '',
Expand All @@ -47,9 +78,9 @@ export default function transformProps(chartProps: ChartProps) {
timeRangeFixed = false,
} = formData;
let { yAxisFormat } = formData;
const { data, from_dttm: fromDatetime, to_dttm: toDatetime } = queryData;
const metricName = metric?.label ? metric.label : metric;
const compareLag = Number(compareLagInput) || 0;
const { data = [], from_dttm: fromDatetime, to_dttm: toDatetime } = queryData;
const metricName = typeof metric === 'string' ? metric : metric.label;
const compareLag = Number(compareLag_) || 0;
const supportTrendLine = vizType === 'big_number';
const supportAndShowTrendLine = supportTrendLine && showTrendLine;
let formattedSubheader = subheader;
Expand All @@ -68,7 +99,8 @@ export default function transformProps(chartProps: ChartProps) {
if (data.length > 0) {
const sortedData = (data as BigNumberDatum[])
.map(d => ({ x: d[TIME_COLUMN], y: d[metricName] }))
.sort((a, b) => b.x - a.x); // sort in time descending order
// sort in time descending order
.sort((a, b) => (a.x !== null && b.x !== null ? b.x - a.x : 0));

bigNumber = sortedData[0].y;
if (bigNumber === null) {
Expand Down Expand Up @@ -103,14 +135,11 @@ export default function transformProps(chartProps: ChartProps) {
}

if (!yAxisFormat && chartProps.datasource && chartProps.datasource.metrics) {
chartProps.datasource.metrics.forEach(
// eslint-disable-next-line camelcase
(metricEntry: { metric_name?: string; d3format: string }) => {
if (metricEntry.metric_name === metric && metricEntry.d3format) {
yAxisFormat = metricEntry.d3format;
}
},
);
chartProps.datasource.metrics.forEach((metricEntry: DatasourceMetric) => {
if (metricEntry.metric_name === metric && metricEntry.d3format) {
yAxisFormat = metricEntry.d3format;
}
});
}

const formatNumber = getNumberFormatter(yAxisFormat);
Expand Down
Loading