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(slider): correct computing of gapRatio #3100

Closed

Conversation

Inselhopper
Copy link
Contributor

While the distance between two labels is too short for text at every label, we find out if we use only ticks with no text (gapRatio > 1) and apply text at any other tick (gapRatio = 2), any third tick (gapRatio = 3) and so on

While the distance between two labels is too short for text at every label,
we find out if we use only ticks with no text (gapRatio > 1)
and apply text at any other tick (gapRatio = 2), any third tick (gapRatio = 3) and so on
@lubber-de
Copy link
Member

lubber-de commented Sep 24, 2024

🤔 Having your changes applied, the sliders end position value is almost never shown anymore, so people have no idea of the maximum possible value.
Also, the shown value totally differs on different sizes, now.
While this usecase could be of interest for others too, it should need a new setting because of the different behavior.

See the very noticable difference here:

Your changes:
https://jsfiddle.net/lubber/6bzcq2hx/3/
gapratioPR

Current FUI
https://jsfiddle.net/lubber/6bzcq2hx/2/
gapratio

@lubber-de lubber-de added type/feat Any feature requests or improvements javascript Pull requests that update Javascript code state/awaiting-reviews Pull requests which are waiting for reviews labels Sep 24, 2024
Copy link
Member

@lubber-de lubber-de left a comment

Choose a reason for hiding this comment

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

See my comment above

@Inselhopper
Copy link
Contributor Author

Inselhopper commented Sep 25, 2024

The current code is buggy. You can see this if you use a prime number. (webm file) https://jsfiddle.net/inselhopper/uczsp3w6/14/
You wrote: "The slider's end position value is almost never shown anymore" → that is true.
The last label is not shown because of the logic, not because of a bug.
How to fix this? Always showing the last label could lead to overlapping text.
Users could use tooltipConfig to show the last value.

You wrote: "Also, the shown value totally differs on different sizes, now." → it seems like that, but not really.

Current.FUI.-.JSFiddle.-.Code.Playground.1.webm

@Inselhopper
Copy link
Contributor Author

Inselhopper commented Sep 30, 2024

Question @prudho: since this part of the slider code is mostly yours: can you have a look please at my changes? See #3103

@prudho
Copy link
Contributor

prudho commented Oct 4, 2024

I wrote this code 5 years ago, and barely touched it since... But here's my two cents:
Yes, the actual code is not optimal. Handling some numbers (primes) will not trigger the autogap adjustment. The idea of using the ratio system is nice, but like @lubber-de said, the slider's end position isn't visible some times, and this disturb me. Your solution should be implemented, but only used when specified by a user (so we keep the actual setting by default).

@lubber-de
Copy link
Member

lubber-de commented Oct 25, 2024

@Inselhopper I adopted your change into a new option value fixed for the autoAdjustLabels Option and added a permanent max slider tick as desired by prudho and me ;) It's implemented into my slider PR #3111 , so i would close this PR in favor of the other .

When autoAdjustLabels: 'fixed' is set, it uses your approach but makes sure the before last label does not overlap to the always shown max tick label
See https://jsfiddle.net/lubber/6bzcq2hx/5/
gapratio2

@lubber-de
Copy link
Member

Closing, as your change was basically implemented by #3111 as mentioned above

@lubber-de lubber-de closed this Oct 31, 2024
@lubber-de lubber-de removed the state/awaiting-reviews Pull requests which are waiting for reviews label Oct 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
javascript Pull requests that update Javascript code type/feat Any feature requests or improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants