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

fix(dashboard): Inoperable dashboard filter slider when range is <= 1 #24012

Closed
wants to merge 2 commits into from

Conversation

jfrancos-mai
Copy link
Contributor

@jfrancos-mai jfrancos-mai commented May 10, 2023

SUMMARY

antd's slider, as used in dashboard filters, has a step size of 1 (antd's default). So when the entire range of numbers is within 1, the slider is inoperable.

A few possible solutions -- I chose option (3):

  1. Set the step size to something static that fixes this particular issue, like .01. This makes things better for this case, but worse for cases where a step size of 1 is sufficient.
  2. Let the user choose a step size in the filter setup dialog. Nice because it makes things explicit, but adds extra UI stuff and users have to make extra decisions.
  3. Come up with a heuristic for choosing a step size. Nice because it works by default, and without any extra work from the user. If we want the user to be able to choose their own step size, that could be a feature rather than a bug fix (and seems to be in-progress over here)

This solution preserves the default behavior where the rightmost digit moves up and down by 1, while accounting for the cases where that 1 ought to be after the decimal point. The calculated step-size is the largest such number (but not larger than 1) that accommodates the minimum number of steps we'd want to have (MIN_NUM_STEPS)

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Screen.Recording.2023-05-10.at.2.20.35.PM.mov
Screen.Recording.2023-05-10.at.2.21.15.PM.mov

TESTING INSTRUCTIONS

How to reproduce the bug

  1. Create a chart using a dataset that has a column whose values are within a range of 1. E.g. a column all of whose values are between 0 and 1.
    1a. If you don't have such a dataset, edit a dataset using "calculated columns" to create one.
  2. Create a dashboard that uses that chart.
  3. Add a numerical range filter to that dashboard, connected to the column in step (1).
  4. Try to use the slider

ADDITIONAL INFORMATION

Fixes #24010

  • 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

@jfrancos-mai jfrancos-mai changed the title Jaf/antd slider step fix(dashboard): Inoperable dashboard filter slider when range is <= 1 May 10, 2023
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Congrats on making your first PR and thank you for contributing to Superset! 🎉 ❤️

We hope to see you in our Slack community too! Not signed up? Use our Slack App to self-register.

@codecov
Copy link

codecov bot commented May 12, 2023

Codecov Report

Merging #24012 (bfd8219) into master (290920c) will increase coverage by 0.43%.
The diff coverage is 68.17%.

❗ Current head bfd8219 differs from pull request most recent head f10cb87. Consider uploading reports for the commit f10cb87 to get more accurate results

@@            Coverage Diff             @@
##           master   #24012      +/-   ##
==========================================
+ Coverage   67.70%   68.13%   +0.43%     
==========================================
  Files        1918     1941      +23     
  Lines       74126    75310    +1184     
  Branches     8052     8166     +114     
==========================================
+ Hits        50187    51314    +1127     
- Misses      21886    21907      +21     
- Partials     2053     2089      +36     
Flag Coverage Δ
hive ?
javascript 54.55% <68.17%> (+0.66%) ⬆️

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

Impacted Files Coverage Δ
...ackages/superset-ui-chart-controls/src/fixtures.ts 100.00% <ø> (ø)
...chart-controls/src/shared-controls/dndControls.tsx 58.33% <ø> (ø)
...d/packages/superset-ui-chart-controls/src/types.ts 100.00% <ø> (ø)
...s/superset-ui-core/src/components/SafeMarkdown.tsx 85.71% <0.00%> (+19.04%) ⬆️
...ackages/superset-ui-core/src/utils/featureFlags.ts 100.00% <ø> (ø)
...s/legacy-plugin-chart-calendar/src/controlPanel.ts 50.00% <ø> (ø)
...nd/plugins/legacy-preset-chart-nvd3/src/NVD3Vis.js 0.00% <ø> (ø)
...rts/src/BigNumber/BigNumberTotal/transformProps.ts 0.00% <0.00%> (ø)
...lugin-chart-echarts/src/BigNumber/BigNumberViz.tsx 0.00% <0.00%> (ø)
...plugin-chart-echarts/src/BoxPlot/transformProps.ts 55.00% <ø> (ø)
... and 64 more

... and 161 files with indirect coverage changes

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

@rusackas
Copy link
Member

Pinged a couple of reviewers, but I'm a little nervous that this might cause as many problems as it solves. If your data has a range of integers from 1-10, for example, then this would cause 20 steps in that range, and may cause queries to fail.

If this is intended specifically for values between 0 and 1, maybe an agreeable compromise is:
const step = max <= 1 ? stepHeuristic(min, max) : 1;

@jfrancos-mai
Copy link
Contributor Author

Pinged a couple of reviewers, but I'm a little nervous that this might cause as many problems as it solves. If your data has a range of integers from 1-10, for example, then this would cause 20 steps in that range, and may cause queries to fail.

If this is intended specifically for values between 0 and 1, maybe an agreeable compromise is: const step = max <= 1 ? stepHeuristic(min, max) : 1;

Hi @rusackas, happy to make that adjustment but a couple questions about it:

const step = max <= 1 ? stepHeuristic(min, max) : 1;

Did you mean:
const step = max - min <= 1 ? stepHeuristic(min, max) : 1;?

Also, I do like

const step = max - min <= 1 ? stepHeuristic(min, max) : 1;

but wondering if you wouldn't prefer

const step = max - min <= 1 ? stepHeuristic(min, max) : undefined;

(where undefined should result in antd's default value of 1). I agree that using 1 instead of undefined is better, as explicit is better than implicit, but maybe it's better to make that change as part of a separate issue?

@michael-s-molina
Copy link
Member

Thank you for the PR @jfrancos-mai! I think the explicit version is preferable:

const step = max - min <= 1 ? stepHeuristic(min, max) : 1;

return Math.min(1, parseFloat(normalizedStepSize));
};

const step = stepHeuristic(min, max);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const step = stepHeuristic(min, max);
const step = max - min <= 1 ? stepHeuristic(min, max) : 1;

@rusackas
Copy link
Member

@jfrancos-mai just wondering if you still want to follow through on this PR. I hope so, but if not, maybe we should close it, or at least convert it to a draft if you want to come back to it at some point in the future.

@michael-s-molina
Copy link
Member

Closing in favor of #27271.

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.

Inoperable dashboard filter slider when range is <= 1
3 participants