Skip to content
This repository has been archived by the owner on Dec 10, 2021. It is now read-only.

feat(core): add orderyby shared control #1122

Merged
merged 1 commit into from
May 19, 2021
Merged

feat(core): add orderyby shared control #1122

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/superset-ui-chart-controls/src/shared-controls/dndControls.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ export const dnd_adhoc_metric: SharedControlConfig<'DndMetricSelect'> = {
default: (c: Control) => mainMetric(c.savedMetrics),
};

export const dnd_timeseries_limit_metric: SharedControlConfig<'DndMetricSelect'> = {
export const dnd_sort_by: SharedControlConfig<'DndMetricSelect'> = {
type: 'DndMetricSelect',
label: t('Sort by'),
default: null,
Expand Down
7 changes: 4 additions & 3 deletions packages/superset-ui-chart-controls/src/shared-controls/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ import {
dnd_adhoc_filters,
dnd_adhoc_metric,
dnd_adhoc_metrics,
dnd_timeseries_limit_metric,
dnd_sort_by,
dndColumnsControl,
dndEntity,
dndGroupByControl,
Expand Down Expand Up @@ -339,7 +339,7 @@ const limit: SharedControlConfig<'SelectControl'> = {
),
};

const timeseries_limit_metric: SharedControlConfig<'MetricsControl'> = {
const sort_by: SharedControlConfig<'MetricsControl'> = {
type: 'MetricsControl',
label: t('Sort By'),
default: null,
Expand Down Expand Up @@ -481,7 +481,8 @@ const sharedControls = {
time_range,
row_limit,
limit,
timeseries_limit_metric: enableExploreDnd ? dnd_timeseries_limit_metric : timeseries_limit_metric,
timeseries_limit_metric: enableExploreDnd ? dnd_sort_by : sort_by,
orderby: enableExploreDnd ? dnd_sort_by : sort_by,
Comment on lines +484 to +485
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this control relates to ordering by metric (as opposed to ordering by e.g. a column name), perhaps we should rename orderby to orderby_metric?

Copy link
Contributor Author

@zhaoyongjie zhaoyongjie May 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no problem at all.

But this control is just for declaring the orderby clause in the SQL, and I guess if we add dimension(column) sorting, we'll probably do it in this control too.

The name of this control, which is also the name of the query object, makes it easier to use.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good points - if we don't think there will be a need for separate ordering controls for 1) only columns 2) only metrics 3) columns and metrics, then this is probably the right solution for now 👍

series: enableExploreDnd ? dndSeries : series,
entity: enableExploreDnd ? dndEntity : entity,
x,
Expand Down