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.

Sign up for GitHub

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: Refactor deck.gl plugins to Typescript #24933

Merged
merged 6 commits into from
Aug 28, 2023
Merged

chore: Refactor deck.gl plugins to Typescript #24933

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d2c1cb0
Convert files to typescript
kgabryje Jul 27, 2023
23a40f1
remove animatable
kgabryje Aug 2, 2023
cce87ef
categorical container
kgabryje Aug 2, 2023
494125e
Refactor layers and utils
kgabryje Aug 2, 2023
901c1ff
Change lodash import
kgabryje Aug 9, 2023
1c96206
Update lastUpdate type
kgabryje Aug 28, 2023
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
2,597 changes: 1,477 additions & 1,120 deletions superset-frontend/package-lock.json
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion superset-frontend/packages/superset-ui-chart-controls/src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ export interface Dataset {
main_dttm_col: string;
// eg. ['["ds", true]', 'ds [asc]']
order_by_choices?: [string, string][] | null;
time_grain_sqla?: string;
time_grain_sqla?: [string, string][];
Copy link
Member

@michael-s-molina michael-s-molina Aug 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think this change is incorrect. If you search for time_grain_sqla in the codebase you'll see it's a string in many places outside the DeckGL plugin.

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 think that in the context of Dataset it actually should be an array. It doesn't really make sense for a dataset to have a single value for time grain, it should rather be a list of supported time grains. Also, if you look at Redux, that's what time_grain_sqla is.

image

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I think you're right! I was thinking about form_data but that makes sense in the context of the Dataset. That's the problem with bad naming.

granularity_sqla?: string;
datasource_name: string | null;
name?: string;
Expand Down
2 changes: 1 addition & 1 deletion superset-frontend/packages/superset-ui-chart-controls/test/utils/columnChoices.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ describe('columnChoices()', () => {
metrics: [],
type: DatasourceType.Table,
main_dttm_col: 'test',
time_grain_sqla: 'P1D',
time_grain_sqla: [],
columns: [
{
column_name: 'fiz',
Expand Down
2 changes: 1 addition & 1 deletion superset-frontend/packages/superset-ui-chart-controls/test/utils/defineSavedMetrics.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ describe('defineSavedMetrics', () => {
],
type: DatasourceType.Table,
main_dttm_col: 'test',
time_grain_sqla: 'P1D',
time_grain_sqla: [],
columns: [],
verbose_map: {},
column_formats: {},
Expand Down
8 changes: 6 additions & 2 deletions superset-frontend/plugins/legacy-preset-chart-deckgl/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,7 @@
"d3-array": "^1.2.4",
"d3-color": "^1.4.1",
"d3-scale": "^3.0.0",
"deck.gl": "8.5.2",
"jquery": "^3.4.1",
Copy link
Member

Choose a reason for hiding this comment

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

that's nice. :)

"deck.gl": "8.8.27",
"lodash": "^4.17.15",
"moment": "^2.20.1",
"mousetrap": "^1.6.1",
Expand All @@ -42,6 +41,11 @@
"urijs": "^1.19.8",
"xss": "^1.0.10"
},
"devDependencies": {
"@types/mapbox__geojson-extent": "^1.0.0",
"@types/underscore": "^1.11.6",
"@types/urijs": "^1.19.19"
},
"peerDependencies": {
"@superset-ui/chart-controls": "*",
"@superset-ui/core": "*",
Expand Down
117 changes: 0 additions & 117 deletions superset-frontend/plugins/legacy-preset-chart-deckgl/src/AnimatableDeckGLContainer.jsx
View file
Open in desktop

This file was deleted.

Loading