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.

Sign up for GitHub

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

Jump to bottom

[core] feat: allow sliders to specify custom label values #4308

Merged
merged 8 commits into from
Oct 2, 2020
Merged

[core] feat: allow sliders to specify custom label values #4308

merged 8 commits into from
Oct 2, 2020

Conversation

cybae0804
Copy link
Contributor

Implements #4297

Checklist

  • Includes tests
  • Update documentation

Changes proposed in this pull request:

Allows labelStepSize to receive an array of specific label positions.

Reviewers should focus on:

First contribution, not sure if I updated the docs properly.

Screenshot

@cybae0804 cybae0804 changed the title Allows labelStepSize to receive an array of specific label positions #4306 Allows labelStepSize to receive an array of specific label positions Sep 3, 2020
@adidahiya adidahiya self-requested a review September 3, 2020 19:34
@cybae0804
Copy link
Contributor Author

Updated as requested.

Some things to note

  • The default for labelStepSize is set to inferred 1 in the docs, because as requested, it's dynamically set.
  • The default for labelStepSize and labelValues are set to null, but in hindsight maybe it would be fine to leave it as undefined. Let me know if I need to change that.
  • The original loop has been significantly simplified to work off of just the label values, and I tested pretty extensively and it all seems to work.

adidahiya
adidahiya suggested changes Oct 2, 2020
View reviewed changes
Copy link
Contributor

@adidahiya adidahiya left a comment

Choose a reason for hiding this comment

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

looking good, just minor comments

packages/core/src/components/slider/multiSlider.tsx Outdated Show resolved Hide resolved
packages/core/src/components/slider/multiSlider.tsx Outdated Show resolved Hide resolved
packages/core/src/components/slider/multiSlider.tsx Outdated Show resolved Hide resolved
packages/core/src/components/slider/multiSlider.tsx Outdated Show resolved Hide resolved
packages/core/src/components/slider/multiSlider.tsx Outdated Show resolved Hide resolved
packages/core/src/components/slider/multiSlider.tsx Outdated Show resolved Hide resolved
packages/core/src/components/slider/multiSlider.tsx Outdated Show resolved Hide resolved
packages/core/test/slider/multiSliderTests.tsx Outdated Show resolved Hide resolved
packages/core/test/slider/multiSliderTests.tsx Show resolved Hide resolved
@cybae0804
Copy link
Contributor Author

Updated with all your suggestions

@adidahiya adidahiya merged commit a0a8fe1 into palantir:develop Oct 2, 2020
@adidahiya adidahiya changed the title Allows labelStepSize to receive an array of specific label positions [core] feat: allow sliders to specify custom label values Oct 2, 2020
@cybae0804 cybae0804 deleted the cb/array-label-step-size branch October 2, 2020 19:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adidahiya adidahiya adidahiya approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@cybae0804 @adidahiya