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

feat(chart): Add <ChartDataProvider /> #120

Merged
merged 33 commits into from
Mar 19, 2019
Merged

Conversation

williaster
Copy link
Contributor

@williaster williaster commented Mar 16, 2019

🏆 Enhancements

This PR adds a new <ChartDataProvider /> component to @superset-ui/charts package, with storybook demo for a handful of vis plugins (new + old). This is essentially a convenience React component around ChartClient with an API inspired by the Apollo <Query /> component:

<ChartDataProvider 
  client={SupersetClient} 
  formData={formData} 
  onLoaded={onLoaded} 
  onError={onError}
>
  {({ loading, payload, error }) => (
    {loading && <Loader />}

    {error && <ErrorMessage error={error} />}

    {payload && (
      <SuperChart
        chartType={payload.formData.viz_type}
        chartProps={
          new ChartProps({
            formData: payload.formData,
            height,
            payload: payload.queryData
            width,
          })
        }
      />
    )}
  )}
</ChartDataProvider>

ChartDataProvider

The PR makes a few tweaks which were needed to get things working

  • [SupersetClient] 🐛 Fixes a reference in the SupersetClient public interface where request referenced get
  • [ChartClient]
    • Fixes some types in ChartClient that were giving me issues when using them (such as SliceIdAndOrFormData)
  • [SuperChart]
    • Removes the remaining .jsx test file and uses @ts-ignore for that test in the .tsx file

Remaining TODO

  • Update README to final API
  • verify 100% test coverage
  • separate mocks/formData into separate files I think given that this is a fixture file now, it's fine in one file

@kristw @conglei @xtinec

@williaster williaster added the reviewable Ready for review label Mar 16, 2019
@williaster williaster requested a review from a team as a code owner March 16, 2019 00:19
@williaster
Copy link
Contributor Author

Still have some todos, but I think reviewable enough/the API is stable.

@williaster williaster added the #enhancement New feature or request label Mar 17, 2019
@codecov
Copy link

codecov bot commented Mar 18, 2019

Codecov Report

Merging #120 into master will decrease coverage by 0.93%.
The diff coverage is 88.67%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #120      +/-   ##
==========================================
- Coverage     100%   99.06%   -0.94%     
==========================================
  Files          74       76       +2     
  Lines         914      964      +50     
  Branches      220      233      +13     
==========================================
+ Hits          914      955      +41     
- Misses          0        6       +6     
- Partials        0        3       +3
Impacted Files Coverage Δ
...ackages/superset-ui-chart/src/models/ChartProps.ts 100% <ø> (ø) ⬆️
...ckages/superset-ui-chart/src/models/ChartPlugin.ts 100% <100%> (ø) ⬆️
...ckages/superset-ui-chart/test/fixtures/formData.ts 100% <100%> (ø)
...kages/superset-ui-connection/src/SupersetClient.ts 100% <100%> (ø) ⬆️
...kages/superset-ui-chart/src/clients/ChartClient.ts 81.81% <57.14%> (-18.19%) ⬇️
...rset-ui-chart/src/components/ChartDataProvider.tsx 92.68% <92.68%> (ø)

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 34581f3...8392736. Read the comment docs.

@codecov
Copy link

codecov bot commented Mar 18, 2019

Codecov Report

Merging #120 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #120   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          74     76    +2     
  Lines         914    958   +44     
  Branches      220    229    +9     
=====================================
+ Hits          914    958   +44
Impacted Files Coverage Δ
...ackages/superset-ui-chart/src/models/ChartProps.ts 100% <ø> (ø) ⬆️
...kages/superset-ui-chart/src/clients/ChartClient.ts 100% <100%> (ø) ⬆️
...rset-ui-chart/src/components/ChartDataProvider.tsx 100% <100%> (ø)
...kages/superset-ui-connection/src/SupersetClient.ts 100% <100%> (ø) ⬆️
...ckages/superset-ui-chart/test/fixtures/formData.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 34581f3...c5a5010. Read the comment docs.

@codecov
Copy link

codecov bot commented Mar 19, 2019

Codecov Report

Merging #120 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #120   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          74     76    +2     
  Lines         914    958   +44     
  Branches      220    229    +9     
=====================================
+ Hits          914    958   +44
Impacted Files Coverage Δ
...ackages/superset-ui-chart/src/models/ChartProps.ts 100% <ø> (ø) ⬆️
...kages/superset-ui-chart/src/clients/ChartClient.ts 100% <100%> (ø) ⬆️
...rset-ui-chart/src/components/ChartDataProvider.tsx 100% <100%> (ø)
...kages/superset-ui-connection/src/SupersetClient.ts 100% <100%> (ø) ⬆️
...ckages/superset-ui-chart/test/fixtures/formData.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 34581f3...11f7227. Read the comment docs.

@williaster
Copy link
Contributor Author

williaster commented Mar 19, 2019

all right this should be good to go now:

  • need to fix @superset-ui/core declaration in @superset-ui/chart (separate PR)
  • moved the declaration of json-bigint types to root/types directory because they were needed in @superset-ui/connection + /chart
  • added tests to ChartClient for legacy API calls
  • set buildQuery=undefined by default so TS is clean
  • fixed typos (thank you! 🙏 ) + updated readme.
  • got up to 💯 % coverage

Copy link
Contributor

@kristw kristw left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you for all the work.

@williaster williaster merged commit ade9dbe into master Mar 19, 2019
@delete-merged-branch delete-merged-branch bot deleted the chris--DataProvider branch March 19, 2019 21:58
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
#enhancement New feature or request reviewable Ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants