-
Notifications
You must be signed in to change notification settings - Fork 14k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: implement drill by table #23603
Conversation
superset-frontend/src/components/Chart/DrillBy/DrillByModal.tsx
Outdated
Show resolved
Hide resolved
superset-frontend/src/components/Chart/DrillBy/DrillByModal.tsx
Outdated
Show resolved
Hide resolved
superset-frontend/src/components/Chart/DrillBy/DrillByModal.tsx
Outdated
Show resolved
Hide resolved
superset-frontend/src/components/Chart/DrillBy/DrillByModal.tsx
Outdated
Show resolved
Hide resolved
superset-frontend/src/components/Chart/DrillBy/DrillByModal.tsx
Outdated
Show resolved
Hide resolved
2b649d7
to
7af5c4a
Compare
}: DrillByModalProps) { | ||
const theme = useTheme(); | ||
const [chartDataResult, setChartDataResult] = useState<QueryData[]>(); | ||
const [drillBy, setDrillBy] = useState<DrillByType>(DrillByType.Chart); |
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.
Could you change the name to sth more descriptive? Like for example displayMode
or drillByDisplayMode
?
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.
Looks great! Added just 1 non-blocking nit
</Radio.Button> | ||
</Radio.Group> | ||
</div> | ||
{drillBy === DrillByType.Chart && chartDataResult && ( |
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.
I think we shouldn't check for chartDataResult
here because then we won't be displaying the loading spinner from DrillByChart
. So we should either remove that check here or move the loading spinner to this component
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.
We probably should display the spinner for Table mode too
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.
i agreed . i will move the spinner to the modal component
Codecov Report
@@ Coverage Diff @@
## master #23603 +/- ##
=======================================
Coverage 67.70% 67.71%
=======================================
Files 1918 1918
Lines 74133 74165 +32
Branches 8052 8056 +4
=======================================
+ Hits 50193 50219 +26
Misses 21887 21887
- Partials 2053 2059 +6
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
SUMMARY
User should be able to see the drill by results either as a chart (the same visualization type as the original chart) or as a table.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION