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

feat(legacy-preset-chart-nvd3): subject NVD3 Bar chart sort by #947

Merged
merged 2 commits into from
Feb 12, 2021

Conversation

maloun96
Copy link
Contributor

@maloun96 maloun96 commented Feb 10, 2021

Add control for sort by "asc" or "desc"

Associated with: apache/superset#13049

AFTER

DEV.Vaccine.Ca.mp4

@maloun96 maloun96 requested a review from a team as a code owner February 10, 2021 09:44
@vercel
Copy link

vercel bot commented Feb 10, 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/bgrxi26ef
✅ Preview: https://superset-ui-git-fork-maloun96-distbarsort.superset.now.sh

Copy link
Contributor

@villebro villebro left a comment

Choose a reason for hiding this comment

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

I think we should default to no ordering, and only opt-in to sorting, both ascending and descending. Perhaps have a "sort bars" control and another "sort descending" one?

@codecov
Copy link

codecov bot commented Feb 10, 2021

Codecov Report

Merging #947 (a6a708a) into master (58ab28e) will increase coverage by 0.18%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #947      +/-   ##
==========================================
+ Coverage   27.49%   27.67%   +0.18%     
==========================================
  Files         412      401      -11     
  Lines        8311     8249      -62     
  Branches     1150     1138      -12     
==========================================
- Hits         2285     2283       -2     
+ Misses       5891     5830      -61     
- Partials      135      136       +1     
Impacted Files Coverage Δ
...gacy-preset-chart-nvd3/src/DistBar/controlPanel.ts 0.00% <ø> (ø)
plugins/plugin-chart-echarts/src/Pie/buildQuery.ts 66.66% <0.00%> (-33.34%) ⬇️
...s/legacy-plugin-chart-country-map/src/countries.js 0.00% <0.00%> (ø)
...superset-ui-core/src/chart/models/ChartMetadata.ts 100.00% <0.00%> (ø)
...egacy-plugin-chart-country-map/src/controlPanel.ts 0.00% <0.00%> (ø)
...ugins/plugin-filter-antd/src/Range/controlPanel.ts
plugins/plugin-filter-antd/src/Range/index.ts
...plugin-filter-antd/src/Select/AntdSelectFilter.tsx
...s/plugin-filter-antd/src/Range/AntdRangeFilter.tsx
...lugins/plugin-filter-antd/src/Select/buildQuery.ts
... and 6 more

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 58ab28e...a6a708a. Read the comment docs.

@maloun96 maloun96 changed the title NVD3 Bar chart sort by feat(legacy-preset-chart-nvd3): subject NVD3 Bar chart sort by Feb 10, 2021
@ktmud
Copy link
Contributor

ktmud commented Feb 11, 2021

Does this re-order bars as well? I'm worried there may be confusion if it doesn't.

@maloun96
Copy link
Contributor Author

Does this re-order bars as well? I'm worried there may be confusion if it doesn't.

@ktmud Actually the sort is working with a row limit, for example, you have a limit of 50, and sort descendent can be different values

@ktmud
Copy link
Contributor

ktmud commented Feb 11, 2021

Yes, I understand how it works, but users might expect it to sort the x-axis.

How about we change "Sort by" to "Sort rows by"?

cc @junlincc

@junlincc
Copy link
Contributor

junlincc commented Feb 11, 2021

I think we should default to no ordering, and only opt-in to sorting, both ascending and descending. Perhaps have a "sort bars" control and another "sort descending" one?

in this case, I agree we should leave it to "no default" since it ties to row limit.
We also discussed about having a 'switch' for ascending and descending. @mihir174

How about we change "Sort by" to "Sort rows by"?

I see your point @ktmud Introducing a new label to one specific chart may create design debt.

  1. let's add tooltip "Sort rows by. changing row limit may affect sorting resulting" something like that?
  2. set no default for this field, and tag it for design-revisit

I know changing back & forth can be tough, thanks for putting up with us😆! @maloun96

@villebro villebro merged commit 51e7ced into apache-superset:master Feb 12, 2021
ktmud pushed a commit that referenced this pull request Feb 18, 2021
* feat(legacy-preset-chart-nvd3): subject WIP

* feat(legacy-preset-chart-nvd3): subject Add sort by for bar chart
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants