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

feat(legacy-plugin-chart-big-number): add control panel config for the BigNumber charts #419

Merged
merged 9 commits into from
Apr 30, 2020

Conversation

suddjian
Copy link
Member

💔 Breaking Changes

🏆 Enhancements

Allows BigNumber to register its own controls, so that the control configs can be removed from superset.

📜 Documentation

🐛 Bug Fix

🏠 Internal

@suddjian suddjian requested a review from a team as a code owner April 29, 2020 22:18
@vercel
Copy link

vercel bot commented Apr 29, 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/q3z065qxl
✅ Preview: https://superset-ui-git-fork-suddjian-bignumber-controls.superset.now.sh

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.

Nice

@@ -18,6 +18,7 @@
*/
import { t } from '@superset-ui/translation';
import { ChartMetadata, ChartPlugin } from '@superset-ui/chart';
import controlPanel from './controls';
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's name the file controlPanel so it is the same when imported.

D3_TIME_FORMAT_OPTIONS,
D3_FORMAT_DOCS,
D3_FORMAT_OPTIONS,
} from '@superset-ui/chart';
Copy link
Contributor

Choose a reason for hiding this comment

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

Please see #417. May have to change reference after that one is merged.

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's merge that and I'll rebase

@@ -338,6 +341,7 @@ export const timeSeriesSection = [
'of query results',
),
controlSetRows: [
// eslint-disable-next-line react/jsx-key
Copy link
Contributor

@ktmud ktmud Apr 29, 2020

Choose a reason for hiding this comment

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

Can move this to top of the file to disable for the whole file.

And just FYI, VS Code allows you to do this in on click:
image

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'm wary of disabling rules for a whole file, it's safer to do it per-line.

Copy link
Contributor

Choose a reason for hiding this comment

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

For this particular case, I don't understand why the rule needs to be disabled though. I see no real code change and it certainly has passed previous CI check?

Copy link
Contributor

Choose a reason for hiding this comment

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

previously it was in the temporary-plugins dir which is excluded from CI build and tests

@codecov
Copy link

codecov bot commented Apr 29, 2020

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #419      +/-   ##
=========================================
- Coverage    3.10%   3.08%   -0.02%     
=========================================
  Files         148     151       +3     
  Lines        5192    5217      +25     
  Branches      272     273       +1     
=========================================
  Hits          161     161              
- Misses       4994    5019      +25     
  Partials       37      37              
Impacted Files Coverage Δ
plugins/big-number/src/BigNumber/controlPanel.tsx 0.00% <0.00%> (ø)
plugins/big-number/src/BigNumber/index.ts 0.00% <ø> (ø)
plugins/big-number/src/BigNumberTotal/index.ts 0.00% <ø> (ø)
plugins/big-number/src/sharedControls.ts 0.00% <0.00%> (ø)
...gins/legacy-preset-chart-nvd3/src/NVD3Controls.tsx 0.00% <ø> (ø)

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 e1700c2...344257b. Read the comment docs.

@suddjian
Copy link
Member Author

Addressed PR feedback

@@ -37,6 +37,7 @@
"peerDependencies": {
"@superset-ui/chart": "^0.13.0",
"@superset-ui/color": "^0.13.0",
"@superset-ui/control-utils": "^0.0.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be ^0.13.0 now.

@kristw kristw changed the title feat: Add control panel config for the BigNumber charts feat(legacy-plugin-chart-big-number): add control panel config for the BigNumber charts Apr 30, 2020
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.

just fixed the version for you. will merge now

@kristw kristw merged commit 590c4f4 into apache-superset:master Apr 30, 2020
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

3 participants