Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(Boxplot): Allow configuration of y-axis range #24380

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dinesh-zemoso
Copy link

@dinesh-zemoso dinesh-zemoso commented Jun 13, 2023

SUMMARY

Added a range slider for Box plot parellel to Y-axis where user can select certain range on the Y-axis
Here is the discussion Link : #16745

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@dinesh-zemoso dinesh-zemoso changed the title Initial commit [Boxplot] Allow configuration of y-axis range Jun 13, 2023
@john-bodley
Copy link
Member

@dinesh-zemoso could you please add some before/after screenshots to help reviewers better grok the nature of the change?

@codecov
Copy link

codecov bot commented Jun 14, 2023

Codecov Report

Merging #24380 (5cf064e) into master (e45be6a) will increase coverage by 0.76%.
The diff coverage is 93.13%.

❗ Current head 5cf064e differs from pull request most recent head 6b311f8. Consider uploading reports for the commit 6b311f8 to get more accurate results

@@            Coverage Diff             @@
##           master   #24380      +/-   ##
==========================================
+ Coverage   68.31%   69.07%   +0.76%     
==========================================
  Files        1957     1903      -54     
  Lines       75596    74541    -1055     
  Branches     8222     8111     -111     
==========================================
- Hits        51640    51487     -153     
+ Misses      21848    20942     -906     
- Partials     2108     2112       +4     
Flag Coverage Δ
javascript 55.62% <75.00%> (+0.89%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...d/packages/superset-ui-chart-controls/src/types.ts 100.00% <ø> (ø)
...ges/superset-ui-core/src/query/buildQueryObject.ts 100.00% <ø> (ø)
...packages/superset-ui-core/src/query/types/Query.ts 100.00% <ø> (ø)
...set-ui-core/src/ui-overrides/ExtensionsRegistry.ts 100.00% <ø> (ø)
...ackages/superset-ui-core/src/utils/featureFlags.ts 100.00% <ø> (ø)
...preset-chart-deckgl/src/layers/Scatter/Scatter.jsx 0.00% <0.00%> (ø)
...plugins/legacy-preset-chart-nvd3/src/Area/index.js 66.66% <ø> (ø)
.../plugins/legacy-preset-chart-nvd3/src/Bar/index.js 66.66% <ø> (ø)
...gins/legacy-preset-chart-nvd3/src/DistBar/index.js 66.66% <ø> (ø)
...plugins/legacy-preset-chart-nvd3/src/Line/index.js 66.66% <ø> (ø)
... and 102 more

... and 103 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@rusackas rusackas changed the title [Boxplot] Allow configuration of y-axis range feat(Boxplot): Allow configuration of y-axis range Aug 19, 2024
@rusackas
Copy link
Member

/testenv up

@rusackas
Copy link
Member

It still seems quite possible to get this across the finish line. The screenshots requested would sure help. I'm not sure if the ephemeral environment will work or not, but we shall see. Meanwhile, if @kgabryje (or anyone he knows) has time to review and test this locally, maybe we can get it in.

{
name: 'show_range_filter',
config: {
type: 'SelectControl',
Copy link
Member

Choose a reason for hiding this comment

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

Can we make it a checkbox like in other Echarts?
Example from bar chart:

[
          {
            name: 'zoomable',
            config: {
              type: 'CheckboxControl',
              label: t('Data Zoom'),
              default: zoomable,
              renderTrigger: true,
              description: t('Enable data zooming controls'),
            },
          },
        ],


const eventHandlers = allEventHandlers(props);
if (formData.showRangeFilter) {
echartOptions.dataZoom = [
Copy link
Member

Choose a reason for hiding this comment

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

We should move this logic to transformProps file

@u35253
Copy link

u35253 commented Dec 4, 2024

Configurable Y-Axis Range in Box Plots would be quite welcome.

That way, if two box plots placed side by side but contain different data ranges within their data (inevitable), the charts could be manually configured to have the same meaningful Y-axis range, so that the chart contents could be visually compared directly.

(I wonder if there is a generalized way to add this feature; all charts with a Y-Axis, such as Bar Charts and Line Charts, can also benefit from a manual Y-axis range.)

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

Successfully merging this pull request may close these issues.

6 participants