feat(dashboards): sorting for table widget visualization #94570
Conversation
github-actions
bot
added
the
Scope: Frontend
❌ 10 Tests Failed:
View the top 3 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
I agree with Cursor's first comment, but I think this is something that can be addressed in a separate PR since it also affects the viewer modal. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| } else if (locationSort?.field === sortColumn) { | ||
| direction = locationSort.kind; | ||
| } | ||
| const nextDirection = direction === 'desc' ? 'asc' : 'desc'; |
There was a problem hiding this comment.
i don't think this is necessary to put here, this can probably be moved to inside the onClick function. This will keep calculating on every render
| const widgetCopy = cloneDeep({ | ||
| ...widget, | ||
| }); |
There was a problem hiding this comment.
| const widgetCopy = cloneDeep({ | |
| ...widget, | |
| }); | |
| const widgetCopy = cloneDeep(widget); |
| }); | ||
| if (widgetCopy.queries[0]) { | ||
| const direction = | ||
| sort.kind === 'desc' && widget.widgetType !== WidgetType.ISSUE ? '-' : ''; |
There was a problem hiding this comment.
is this gonna be an issue with some of the issue widget fields? (referencing one of your previous prs) Just curious, not sure if that needs to be taken into account here.
There was a problem hiding this comment.
Should be fine, issues fields omit the '-' regardless of order that it is sorted by. So when updating the widget query, we want to make sure it's never included
| // This is to match the widget viewer modal, which always displays the arrow pointing down | ||
| const sort: Sort = { | ||
| field: decodeSorts(widget.queries[0]?.orderby)[0]?.field || '', | ||
| kind: 'desc', |
There was a problem hiding this comment.
same comment as above about the issues widget default sort stuff.
There was a problem hiding this comment.
This would be purely visual since that widget.widgetType !== WidgetType.ISSUE always removes the '-'.
I'm honestly thinking it might be better to omit sorting for issue widgets for now/make a separate PR to address it. Might want to get design's opinion as well since the arrow direction lowkey makes no sense
There was a problem hiding this comment.
I know I keep hammering on you about splitting PRs but this PR would be even better if it was two PRs! One to introduce the new functionality, and a separate one to integrate into Dashboards, once the functionality is all approved and merged.
Otherwise, I might forget to ask that the new sorting functionality has specs, for example, and/or you'll need to make more changes if I ask for a prop to be renamed, or whatever!
In any case, left a few comments about how it works, even though I think overall it LGTM 👍🏻
| * A callback function that is invoked after a user clicks a sortable column header and overrides default behaviour of navigating | ||
| * @param sort `Sort` object contain the `field` and `kind` ('asc' or 'desc') | ||
| */ | ||
| onColumnSortChange?: (sort: Sort) => void; |
There was a problem hiding this comment.
| onColumnSortChange?: (sort: Sort) => void; | |
| onSortChange?: (sort: Sort) => void; |
onSortChange is nicer here, it's shorter and matches sort. Only columns can be sorted, right?
| @@ -73,10 +75,19 @@ interface TableWidgetVisualizationProps { | |||
| * @param meta The full table metadata | |||
| */ | |||
| makeBaggage?: BaggageMaker; | |||
| /** | |||
| * A callback function that is invoked after a user clicks a sortable column header and overrides default behaviour of navigating | |||
There was a problem hiding this comment.
| * A callback function that is invoked after a user clicks a sortable column header and overrides default behaviour of navigating | |
| * A callback function that is invoked after a user clicks a sortable column header. If omitted, clicking a column header updates the sort in the URL |
| @@ -136,6 +149,7 @@ export function TableWidgetVisualization(props: TableWidgetVisualizationProps) { | |||
| })); | |||
|
|
|||
| const {data, meta} = tableData; | |||
| const locationSort = decodeSorts(location?.query?.sort)[0]; | |||
There was a problem hiding this comment.
👀 I don't mind that it only supports a single sort for now, but it'd be good to document this limitation in the stories!
There was a problem hiding this comment.
Oh interesting, is there any use case for multiple sorts right now? But will update the stories.
There was a problem hiding this comment.
I don't know offhand if any of our UIs support it! I don't even know how you'd specific which sort is first and which one is second.
| <p> | ||
| By default, column fields are assumed to be not sortable. To enable sorting, | ||
| pass the | ||
| <code>columns</code> prop with the field <code>sortable</code> set to true. Ex. |
There was a problem hiding this comment.
This is the nittest of nits but most of the documentation uses Chicago-style "e.g.,"instead of"Ex."
static/app/views/dashboards/widgets/tableWidget/tableWidgetVisualization.tsx
Outdated
Show resolved
Hide resolved
| // The Sort type | ||
| export type Sort = { | ||
| field: string; | ||
| kind: 'asc' | 'desc'; | ||
| }; | ||
|
|
||
| // Basic Example | ||
| function onColumnSortChange(sort: Sort) { | ||
| setSort(sort) | ||
| } |
There was a problem hiding this comment.
No need to explain the Sort type here! IMO would be better to give a more illustrative example. e.g.,
| // The Sort type | |
| export type Sort = { | |
| field: string; | |
| kind: 'asc' | 'desc'; | |
| }; | |
| // Basic Example | |
| function onColumnSortChange(sort: Sort) { | |
| setSort(sort) | |
| } | |
| <TableWidgetVisualization | |
| columns={[{field: 'count( | |
| sort={{field: 'count( | |
| tableData=... |
As in, have the sample be a whole snippet that explains how the data, the columns, and the sort are specified together
| <p> | ||
| The table will try to automatically parse out the direction from the location | ||
| query parameter and apply the sort direction arrow to the sorted column. | ||
| However, if sorting does not rely on this, or custom sort needs to be used, then | ||
| pass the <code>sort</code> prop to correcly display the sort arrow direction: | ||
| </p> | ||
| <CodeSnippet> | ||
| {`sort={{field: 'count(span.duration)', kind: 'desc'}}`} | ||
| </CodeSnippet> |
There was a problem hiding this comment.
This is a cool default, but would be good to also explain with a fuller example, IMO!
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Bug: Visual Sort Indicator Mismatch
The sort.kind property is hardcoded to 'desc' in IssueWidgetCard, even though the sort field is correctly derived from the widget's orderby query. This causes the visual sort indicator in the UI to always show a descending arrow, regardless of the actual sort direction of the data, misleading users about the current sort state.
static/app/views/dashboards/widgetCard/issueWidgetCard.tsx#L86-L91
sentry/static/app/views/dashboards/widgetCard/issueWidgetCard.tsx
Lines 86 to 91 in cd6f902
Was this report helpful? Give feedback by reacting with 👍 or 👎
### Changes Added sorting functionality to the table widget visualization component. Part one of this splitting this PR: #94570 Will open another PR to integrate it into dashboards.
Changes
Add sorting to table widget visualization. Sorting should now work for widgets on the dashboard and widget preview when editing.
Before
Sorting was not implemented on dashboard widgets before this
After
SORTING.mov