-
Notifications
You must be signed in to change notification settings - Fork 14.3k
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(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)