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/range slider new props #1069

Closed
wants to merge 5 commits into from
Closed

Feat/range slider new props #1069

wants to merge 5 commits into from

Conversation

SebasF1349
Copy link
Member

@SebasF1349 SebasF1349 commented Mar 1, 2023

Before submitting the PR:

  • Does your PR reference an issue? If not, please chat to the team on Discord or GitHub before submission.
  • Did you update and run tests before submission using npm run test?
  • Does your branch follow our naming convention? If not, please amend the branch name using branch -m new-branch-name
  • Did you update documentation related to your new feature or changes?

What does your PR address?

Fixes #1066

msedge_cr7E16pete

Important notes:

  • As discussed, added the option to show number values, custom string values and to opt in for vertical labels (horizontal by default).
  • Apart from that, I also added labelsSteps to allow to don't show labels in every tick (as in the previous pic).
  • The logic has been manually tested a lot, but there are some issues with the styles:
    • I cannot add the styles as Tailwind classes because it doesn't work the same in my end.
    • Vertical labels are not 100% centered in the tick.
    • Similarly, when many ticks (not necessarily labels) are shown, labels are not centered in the ticks as shown below.
      firefox_4gmnq1poTX
  • Need to create automatic tests for the new props.
  • I can update documentation if this is eventually merged.

@vercel
Copy link

vercel bot commented Mar 1, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated
skeleton-docs ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Mar 1, 2023 at 2:43PM (UTC)

@endigo9740
Copy link
Contributor

endigo9740 commented Mar 2, 2023

@SebasF1349 I love the concept of this feature, but I think the way it's being implemented could use it a bit of adjustment. I won't have time to write a proper set of instructions here today, but just wanted to let you know where we are on this. I definitely think this will be a valuable feature though.

We're juggling a few things right now, so expect this sometime early next week. Thanks!

@endigo9740
Copy link
Contributor

endigo9740 commented Mar 7, 2023

@SebasF1349 just wanted to let you know I haven't forgot about this. We may actually opt to hold off for this post v1.0 just so we can take the proper amount of time to implement and test this. If you're ok with that I'll circle back with you next week.

But general idea is I like your idea of being able to feed in the data points, but I'd like to also include a means to automate this. I haven't quite formalized the specifics for how to do this, but general idea is if I have 100 ticks, I'm not going to want to have to manually label each of those. Perhaps this is a second or addition feature that's enabled that basically tells the component "do this for me".

I'll need to prototype a bit, but I will follow up. Thanks again for your patience in the meantime.

@SebasF1349
Copy link
Member Author

No problem! Glad you liked the idea. My pr was definitely not production ready so I knew it will need some work. Also, too much work this last weeks, so completely understandable it gets pushed to after v1. Let me know if I can help in anything regarding this.

@endigo9740
Copy link
Contributor

@SebasF1349 as discussed yesterday I'm closing this, but will refer to this in the ticket when I generate my iteration of this.

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

Successfully merging this pull request may close these issues.

Add labels to ticks in Range Slider
2 participants