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

feat: remove the default Y-axis truncate in bar chart #1450

Merged
merged 3 commits into from
Nov 9, 2021

Conversation

stephenLYZ
Copy link
Contributor

remove the default Y-axis truncate check box to “unchecked” and add a tooltip next to the checkbox saying “It’s not recommended to truncate y-axis in Bar chart”

image

@stephenLYZ stephenLYZ requested a review from a team as a code owner November 2, 2021 04:03
@vercel
Copy link

vercel bot commented Nov 2, 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/3EsXTNymkD5iVeXCkv1NaEi26s24
✅ Preview: https://superset-ui-git-fork-stephenlyz-feat-bar-control-superset.vercel.app

@codecov
Copy link

codecov bot commented Nov 2, 2021

Codecov Report

Merging #1450 (5b5a34f) into master (0ca4f73) will increase coverage by 0.00%.
The diff coverage is 50.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1450   +/-   ##
=======================================
  Coverage   30.62%   30.63%           
=======================================
  Files         499      500    +1     
  Lines       10048    10052    +4     
  Branches     1699     1699           
=======================================
+ Hits         3077     3079    +2     
- Misses       6723     6725    +2     
  Partials      248      248           
Impacted Files Coverage Δ
...-chart-echarts/src/Timeseries/Regular/Bar/index.ts 25.00% <ø> (ø)
...ugins/plugin-chart-echarts/src/Timeseries/types.ts 100.00% <ø> (ø)
...charts/src/Timeseries/Regular/Bar/controlPanel.tsx 50.00% <50.00%> (ø)

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 0ca4f73...5b5a34f. Read the comment docs.

Copy link
Contributor

@zhaoyongjie zhaoyongjie left a comment

Choose a reason for hiding this comment

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

@junlin code LGTM. one nit: should we keep all echart timeseries viz consistent for this control?
cc: @villebro

@villebro
Copy link
Contributor

villebro commented Nov 2, 2021

For context, y-axis truncation was enabled by default only because it was default behavior in the NVD3 charts. I'm personally against truncation of y-axes by default (I think it's ok in certain cases, but should be a conscious decision), so I'd be happy to see truncation being disabled by default in all charts. But I assume there may be community resistance to this, as changing these types of things have historically proven to be a fairly sensitive topic.

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.

Code changes LGTM, but let's think this through once more before merging to make sure we have broad alignment for changing the default behavior.

@junlincc
Copy link
Contributor

junlincc commented Nov 2, 2021

Is the tooltip only showing in bar chart? if both behaviors 1)not set by default 2) showing tooltip are only showing in Bar chart, i think we are good.

@zhaoyongjie
Copy link
Contributor

Is the tooltip only showing in bar chart? if both behaviors 1)not set by default 2) showing tooltip are only showing in Bar chart, i think we are good.

So, Do we change the truncate y-axis control to uncheck in all echart viz?

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.

4 participants