-
Notifications
You must be signed in to change notification settings - Fork 14.6k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
feat(dashboard): Add Drill to Detail modal w/ chart menu + right-click support #20728
feat(dashboard): Add Drill to Detail modal w/ chart menu + right-click support #20728
Conversation
f663ec8
to
54a18c1
Compare
superset-frontend/src/dashboard/components/DatasourceResultsPane/index.tsx
Outdated
Show resolved
Hide resolved
superset-frontend/src/dashboard/components/DatasourceResultsPane/index.tsx
Outdated
Show resolved
Hide resolved
superset-frontend/src/dashboard/components/DatasourceResultsPane/index.tsx
Outdated
Show resolved
Hide resolved
superset-frontend/src/dashboard/components/DatasourceResultsPane/index.tsx
Outdated
Show resolved
Hide resolved
091f022
to
12d3f56
Compare
3ca3f87
to
2e4f5ee
Compare
Codecov Report
@@ Coverage Diff @@
## master #20728 +/- ##
=======================================
Coverage 66.16% 66.16%
=======================================
Files 1773 1773
Lines 67689 67689
Branches 7218 7218
=======================================
Hits 44786 44786
Misses 21059 21059
Partials 1844 1844
Flags with carried forward coverage won't be shown. Click here to find out more. 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
2e4f5ee
to
9b7e22c
Compare
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 left some suggestions. codes look good. I will test it in local. Thanks @codyml
superset-frontend/src/dashboard/components/DatasourceResultsPane/index.tsx
Outdated
Show resolved
Hide resolved
superset-frontend/src/dashboard/components/DatasourceResultsPane/index.tsx
Outdated
Show resolved
Hide resolved
superset-frontend/src/dashboard/components/DatasourceResultsPane/index.tsx
Outdated
Show resolved
Hide resolved
superset-frontend/src/dashboard/components/DrillDetailPane/DrillDetailPane.tsx
Outdated
Show resolved
Hide resolved
superset-frontend/src/dashboard/components/DrillDetailPane/DrillDetailPane.tsx
Outdated
Show resolved
Hide resolved
superset-frontend/src/dashboard/components/DrillDetailPane/DrillDetailPane.tsx
Show resolved
Hide resolved
initialFilters?: BinaryQueryObjectFilterClause[]; | ||
}) { | ||
const theme = useTheme(); | ||
const [pageIndex, setPageIndex] = useState(0); |
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.
const [pageIndex, setPageIndex] = useState(0); | |
const [pageIndex, setPageIndex] = useState(1); |
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'm using 0-indexed page indices in front-end code because the TableView
component does and I felt like consistency with that was more important – that okay?
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.
The TableView
has a property initialPageIndex
, this property means which page is the first page when pagination. BTW, in the pagination context, the index
means page number
instead of index
, so maybe the first page is 1.
superset/superset-frontend/src/components/TableView/TableView.tsx
Lines 118 to 139 in e214e1a
const TableView = ({ | |
columns, | |
data, | |
pageSize: initialPageSize, | |
totalCount = data.length, | |
initialPageIndex, | |
initialSortBy = [], | |
loading = false, | |
withPagination = true, | |
emptyWrapperType = EmptyWrapperType.Default, | |
noDataText, | |
showRowCount = true, | |
serverPagination = false, | |
columnsForWrapText, | |
onServerPagination = () => {}, | |
...props | |
}: TableViewProps) => { | |
const initialState = { | |
pageSize: initialPageSize ?? DEFAULT_PAGE_SIZE, | |
pageIndex: initialPageIndex ?? 0, | |
sortBy: initialSortBy, | |
}; |
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.
Oh great, didn't see that thanks! Will update.
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.
@zhaoyongjie I'm having trouble using initialPageIndex
to tell TableView
to use 1-indexed page numbers: it seems like for server pagination, that parameter is used to set the current page number instead, and TableView
expects the first page index to be 0
. If you can figure out how to do it without rewriting TableView
to expect 1-indexed page numbers, feel free to push a commit.
superset-frontend/src/dashboard/components/DrillDetailPane/index.ts
Outdated
Show resolved
Hide resolved
superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx
Show resolved
Hide resolved
superset-frontend/src/dashboard/components/DrillDetailPane/DrillDetailPane.tsx
Show resolved
Hide resolved
Hi @codyml. We need to account for charts and dashboard configurations when displaying the drilling details panel. One example is the chart below: In this chart, we should consider the controls configuration that impacts the query like I believe the samples pane in Explore should also account for these filters. If I set The drilling details panel should also be influenced by dashboard configurations like To make things more complicated, dashboard filters can come from the filters panel or cross-filters. I don't know if I covered everything. We may have other sources that impact a chart. Tagging some folks to see if I'm missing anything @rusackas @lauderbaugh @zhaoyongjie @kgabryje @geido |
It's not true. The It is a point worth discussing why the original implementation did not reset the The |
Closed to refocus on additional work required given clarified requirements. |
Reopening! |
Can we display the modal with a fixed height/width? I find it weird that it starts small and grows after the results are rendered. |
Dashboard Regression Test LGTM |
621ea73
to
2310b03
Compare
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
Ephemeral environment shutdown and build artifacts deleted. |
how to use in 2.0.0 branch |
Hi @JunTech! This feature wasn't included in the 2.0.0 release, so using it would require running Superset from your own non-release branch that includes this commit (and previous supporting commits), then enabling the |
SUMMARY
This PR introduces a drill-to-detail modal. It adds a "Drill to detail" menu item to dashboard charts' contextual menus and also completes support for opening the modal by right-clicking on chart features. The drill-to-detail modal allows users to inspect the data that is backing a chart from the dashboard that the chart is on, while optionally applying filters based on where in the chart was clicked.
More details:
DRILL_TO_DETAIL
feature flag is enabled.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Menu item:

Modal:

Modal w/ native filters:

TESTING INSTRUCTIONS
Charts supporting right-click:
ADDITIONAL INFORMATION
DRILL_TO_DETAIL
(DASHBOARD_CROSS_FILTERS
optional to test cross-filter support)