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

fix(plugin-chart-word-cloud): make wordcloud take current formdata #428

Merged
merged 2 commits into from
Apr 30, 2020

Conversation

kristw
Copy link
Contributor

@kristw kristw commented Apr 30, 2020

🐛 Bug Fix 1

  • The WordCloud plugin was written with more flexible api than the original word cloud. (Basically, the props are different.)

original wordcloud props

type OriginalProps = {
  width: number,
  height: number,
  rotation: string,
  data: { size: number, text: string } [];,
  sizeRange: number[],
  colorScheme: string,
}

example

{
  colorScheme: 'd3Category10',
  metric: 'sum__num',
  rotation: 'square',
  series: 'name',
  sizeFrom: '10',
  sizeTo: '70',
}

new wordcloud props

type NewProps = {
  width: number;
  height: number;
  rotation?: RotationType;
  data: { [key:string]: any; }[];
  // support a vega-lite / grammar of graphic -style declaration
  // to define encoding for different channels of the word cloud
  // e.g. color, fontSize, fontFamily, fontWeight, text
  encoding?: Partial<WordCloudEncoding>;
}

example

{
  encoding: {
    color: {
      type: 'nominal',
      field: 'name',
      scheme: 'd3Category10',
    },
    fontSize: {
      field: 'sum__num',
      scale: {
        range: [10, 70],
        zero: true,
      },
      type: 'quantitative',
    },
    text: {
      field: 'name',
    },
  },
  rotation: 'square',
}

incubator-superset passes formData to the WordCloud as props. What happen in the app is something equivalent to this.

<WordCloud {...formData} />

In the future the formData can take more advantage of the new WordCloud API. However, at the moment, incubator-superset is still passing the formData in original format to the new WordCloud, so we need transformation of the props to fit the new WordCloud props.

<WordCloud {...transform(formData)} />

🐛 Bug Fix 2

  • Word Cloud layout algorithm also mutates the input, so add cloning step to avoid input mutation.

@kristw kristw requested a review from a team as a code owner April 30, 2020 18:56
@vercel
Copy link

vercel bot commented Apr 30, 2020

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/muvixbjid
✅ Preview: https://superset-ui-git-kristw-fix-wc.superset.now.sh

@codecov
Copy link

codecov bot commented Apr 30, 2020

Codecov Report

Merging #428 into master will decrease coverage by 0.00%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #428      +/-   ##
=========================================
- Coverage    3.08%   3.08%   -0.01%     
=========================================
  Files         151     151              
  Lines        5217    5218       +1     
  Branches      273     273              
=========================================
  Hits          161     161              
- Misses       5019    5020       +1     
  Partials       37      37              
Impacted Files Coverage Δ
...ns/plugin-chart-word-cloud/src/chart/WordCloud.tsx 0.00% <0.00%> (ø)
...lugins/plugin-chart-word-cloud/src/plugin/index.ts 33.33% <ø> (ø)

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 69952fd...01f5eae. Read the comment docs.

@kristw kristw merged commit 80b4c3f into master Apr 30, 2020
@delete-merged-branch delete-merged-branch bot deleted the kristw--fix-wc branch April 30, 2020 20:49
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.

None yet

4 participants