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

Add WordCloud plugin #50

Merged
merged 11 commits into from
Feb 5, 2019
Merged

Add WordCloud plugin #50

merged 11 commits into from
Feb 5, 2019

Conversation

kristw
Copy link
Contributor

@kristw kristw commented Dec 5, 2018 •
edited
Loading

🏆 Enhancements

Add WordCloudChartPlugin

WIP: Need to resolve reactify dependency and add unit tests.

@kristw kristw requested a review from a team as a code owner December 5, 2018 01:48
@kristw kristw added the WIP Work in progress label Dec 5, 2018
@kristw kristw added the #enhancement New feature or request label Dec 6, 2018
@conglei conglei mentioned this pull request Dec 6, 2018
Closed
@williaster williaster mentioned this pull request Dec 6, 2018
Closed
@xtinec xtinec force-pushed the kristw--wordcloud branch 2 times, most recently from deaa621 to 14f1f50 Compare December 13, 2018 00:16
@codecov
Copy link

codecov bot commented Dec 13, 2018 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (master@b7b9524). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##             master    #50   +/-   ##
=======================================
  Coverage          ?   100%           
=======================================
  Files             ?     75           
  Lines             ?    863           
  Branches          ?    194           
=======================================
  Hits              ?    863           
  Misses            ?      0           
  Partials          ?      0
Impacted Files Coverage Δ
...ackages/superset-ui-chart/src/models/ChartProps.ts 100% <ø> (ø)
...ckages/superset-ui-chart/src/models/ChartPlugin.ts 100% <ø> (ø)
...t-ui-plugin-chart-word-cloud/src/transformProps.ts 100% <100%> (ø)
...erset-ui-plugin-chart-word-cloud/src/buildQuery.ts 100% <100%> (ø)

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 b7b9524...17d4534. Read the comment docs.

@xtinec xtinec force-pushed the kristw--wordcloud branch 4 times, most recently from ed970d1 to edf3d30 Compare December 20, 2018 21:01
@xtinec xtinec added reviewable Ready for review and removed WIP Work in progress labels Jan 4, 2019
@xtinec xtinec force-pushed the kristw--wordcloud branch from 26e1fe3 to bd39498 Compare January 4, 2019 01:55
@xtinec
Copy link
Contributor

xtinec commented Jan 4, 2019

Ported the word cloud plugin to TypeScript and added new tests for transformProps. I think this PR is ready for review.

P.S. I think we could handle the post-build script for copying assets like images/thumbnail.png (and other build changes) in a separate PR.

kristw
kristw commented Jan 10, 2019
View reviewed changes
packages/superset-ui-plugin-chart-word-cloud/types/external.d.ts Outdated
@@ -0,0 +1,3 @@
declare module '*.png';
declare module '@superset-ui/color';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

color is now in TS.

kristw
kristw commented Jan 10, 2019
View reviewed changes
packages/superset-ui-plugin-chart-word-cloud/types/external.d.ts Outdated
@@ -0,0 +1,3 @@
declare module '*.png';
declare module '@superset-ui/color';
declare module '@superset-ui/translation';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Has another PR converting translation to TS.

@kristw kristw force-pushed the kristw--wordcloud branch 3 times, most recently from 62071f3 to b801005 Compare January 28, 2019 23:38
@kristw kristw force-pushed the kristw--wordcloud branch from b801005 to 89e2be1 Compare February 5, 2019 01:24
xtinec
xtinec reviewed Feb 5, 2019
View reviewed changes
packages/superset-ui-chart/src/models/ChartPlugin.ts
@@ -37,13 +37,13 @@ export interface ChartPluginConfig {
loadChart?: PromiseOrValueLoader<Function>;
}

export default class ChartPlugin extends Plugin {
export default class ChartPlugin<T extends FormData = FormData> extends Plugin {
Copy link
Contributor

Choose a reason for hiding this comment

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

What does the = FormData do?

Copy link
Contributor

Choose a reason for hiding this comment

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

defaults to that generic?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, default to that generic so all the charts without specific FormData can fallback to it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Aha, got it. That makes sense.

xtinec
xtinec reviewed Feb 5, 2019
View reviewed changes
Copy link
Contributor

@xtinec xtinec left a comment

Choose a reason for hiding this comment

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

Looking good. Just one Q.

williaster
williaster reviewed Feb 5, 2019
View reviewed changes
packages/superset-ui-chart/src/models/ChartPlugin.ts
@@ -13,20 +13,20 @@ const IDENTITY = (x: any) => x;
type PromiseOrValue<T> = Promise<T> | T;
type PromiseOrValueLoader<T> = () => PromiseOrValue<T> | PromiseOrValue<{ default: T }>;

export type BuildQueryFunction = (formData: FormData) => QueryContext;
export type BuildQueryFunction<T extends FormData> = (formData: T) => QueryContext;
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️ this

williaster
williaster reviewed Feb 5, 2019
View reviewed changes
Copy link
Contributor

@williaster williaster left a comment

Choose a reason for hiding this comment

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

lgtm after the WIP items 👏 🎬

@kristw
Copy link
Contributor Author

kristw commented Feb 5, 2019

Umm, ci is throwing error that I don't see locally. Have to figure out what is going on.

@kristw kristw force-pushed the kristw--wordcloud branch from 89e2be1 to e3acbce Compare February 5, 2019 19:00
xtinec
xtinec approved these changes Feb 5, 2019
View reviewed changes
Copy link
Contributor

@xtinec xtinec left a comment

Choose a reason for hiding this comment

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

Code change looks great. I think it is good to 🚢 once we sort out the errors on CI.

@kristw kristw added this to the v0.9.3 milestone Feb 5, 2019
@kristw
Copy link
Contributor Author

kristw commented Feb 5, 2019

CI passes with 100% coverage. Somehow codecov stalled. I will just merge it to unblock other tasks and fix master build.

@kristw kristw merged commit 5afc9b6 into master Feb 5, 2019
@delete-merged-branch delete-merged-branch bot deleted the kristw--wordcloud branch February 5, 2019 21:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@williaster williaster williaster left review comments

@xtinec xtinec xtinec approved these changes

Assignees
No one assigned
Labels
#enhancement New feature or request reviewable Ready for review
Projects
None yet
Milestone
v0.9.3
Development

Successfully merging this pull request may close these issues.
3 participants
@kristw @xtinec @williaster