-
Notifications
You must be signed in to change notification settings - Fork 272
feat(core): expand native filter hook and add chart metadata fields #943
Conversation
…ty / remove native filters
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/superset/superset-ui/o79i4996v |
Codecov Report
@@ Coverage Diff @@
## master #943 +/- ##
==========================================
+ Coverage 27.49% 27.67% +0.18%
==========================================
Files 412 401 -11
Lines 8311 8248 -63
Branches 1150 1137 -13
==========================================
- Hits 2285 2283 -2
+ Misses 5891 5830 -61
Partials 135 135
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few comments
export enum Behaviour { | ||
CROSS_FILTER = 'CROSS_FILTER', | ||
NATIVE_FILTER = 'NATIVE_FILTER', | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I believe the codebase "speaks American", so this should probably be Behavior
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
import React from 'react'; | ||
import { action } from '@storybook/addon-actions'; | ||
import { boolean, withKnobs } from '@storybook/addon-knobs'; | ||
import { SuperChart, getChartTransformPropsRegistry } from '@superset-ui/core'; | ||
import { AntdSelectFilterPlugin } from '@superset-ui/plugin-filter-antd'; | ||
import transformProps from '@superset-ui/plugin-filter-antd/lib/Select/transformProps'; | ||
import data from './data'; | ||
import { withResizableChartDemo } from '../../../../shared/components/ResizableChartDemo'; | ||
import 'antd/dist/antd.css'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a reminder: we need migrate these to the apache/superset
storybook.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -1,3 +1,5 @@ | |||
import { Behavior } from '../types/Base'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would prefer this to be called ChartFilterBehavior
(after first-time seeing it in apache/superset#13052 and being confused of what it is), but looks like I'm late to the party...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It won't necessarily be restricted to filtering behavior, for example drill-downs which are essentially groupbys, but ChartBehavior
could also be ok. Let me know if you want to update this now, otherwise we'll stick to this naming.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's save the update to when we need to expand this functionality.
This PR:
SetExtraFormDataHook
isNativeFilter
metadata flag tobehaviours
property that contains array of implemented behaviours💔 Breaking Changes
🏆 Enhancements
📜 Documentation
🐛 Bug Fix
🏠 Internal