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

Conversation

zhaoyongjie
Copy link
Contributor

@zhaoyongjie zhaoyongjie commented May 18, 2021

🏆 Enhancements
Add a new shared control "orderby" for non-timeseries chart.

related PR: #1112

Now all non-time series chart, are using the "timeseries_limit_metric" control. I would like to introduce a unified sorting control. So that in the future we can have a unified sorting control (sort columns and metrics).

@zhaoyongjie zhaoyongjie requested a review from a team as a code owner May 18, 2021 15:41
@vercel
Copy link

vercel bot commented May 18, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/superset/superset-ui/DNwmffspeAjaDcEhZUKdR9DCYdFm
✅ Preview: https://superset-ui-git-fork-zhaoyongjie-addsortbycontrol-superset.vercel.app

@zhaoyongjie zhaoyongjie force-pushed the add_sortby_control branch from 237b44d to d302f86 Compare May 18, 2021 15:44
@zhaoyongjie zhaoyongjie changed the title Add orderyby shared control feat: add orderyby shared control May 18, 2021
@zhaoyongjie zhaoyongjie changed the title feat: add orderyby shared control feat(core): add orderyby shared control May 18, 2021
@codecov
Copy link

codecov bot commented May 18, 2021

Codecov Report

Merging #1122 (d302f86) into master (7da2a9d) will increase coverage by 0.00%.
The diff coverage is 33.33%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1122   +/-   ##
=======================================
  Coverage   28.98%   28.99%           
=======================================
  Files         462      462           
  Lines        9200     9209    +9     
  Branches     1449     1453    +4     
=======================================
+ Hits         2667     2670    +3     
- Misses       6331     6334    +3     
- Partials      202      205    +3     
Impacted Files Coverage Δ
...chart-controls/src/shared-controls/dndControls.tsx 27.58% <ø> (-0.99%) ⬇️
...et-ui-chart-controls/src/shared-controls/index.tsx 37.63% <33.33%> (-0.41%) ⬇️
...ugins/plugin-chart-echarts/src/Gauge/buildQuery.ts 75.00% <0.00%> (-25.00%) ⬇️
...ntrols/src/components/CertifiedIconWithTooltip.tsx 20.00% <0.00%> (-13.34%) ⬇️
...plugin-chart-echarts/src/Treemap/transformProps.ts 52.00% <0.00%> (-1.07%) ⬇️
plugins/plugin-chart-echarts/src/Treemap/types.ts 100.00% <0.00%> (ø)
...gins/plugin-chart-echarts/src/Pie/controlPanel.tsx 33.33% <0.00%> (ø)
.../plugin-chart-echarts/src/Funnel/transformProps.ts 74.35% <0.00%> (ø)
...s/plugin-chart-echarts/src/Funnel/controlPanel.tsx 66.66% <0.00%> (+16.66%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7da2a9d...d302f86. Read the comment docs.

Comment on lines +484 to +485
timeseries_limit_metric: enableExploreDnd ? dnd_sort_by : sort_by,
orderby: enableExploreDnd ? dnd_sort_by : sort_by,
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

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 👍

@zhaoyongjie zhaoyongjie merged commit 07256ed into apache-superset:master May 19, 2021
@junlincc
Copy link
Contributor

@zhaoyongjie please list all chart that are added this control.

@zhaoyongjie
Copy link
Contributor Author

zhaoyongjie commented May 28, 2021

@zhaoyongjie please list all chart that are added this control.

there is no chart with this control.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants