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(slider): add section labels support #288

Merged
merged 2 commits into from
Sep 17, 2024

Conversation

scharinger
Copy link
Contributor

@scharinger scharinger commented Jul 4, 2024

Describe your changes

We need a slider with sections that have labels and customable track colors.

Checklist before requesting a review

  • I have performed a self-review of my own code
  • I have verified that the code builds perfectly fine on my local system
  • I have added tests that prove my fix is effective or that my feature works
  • I have commented my code, particularly in hard-to-understand areas
  • I have verified that my code follows the style already available in the repository
  • I have made corresponding changes to the documentation

image

@scharinger scharinger requested a review from a team as a code owner July 4, 2024 09:01
@scharinger scharinger force-pushed the add-section-labels branch 10 times, most recently from 1801cb1 to 3d7db36 Compare July 4, 2024 15:39
@scharinger scharinger marked this pull request as draft July 4, 2024 15:41
@scharinger scharinger force-pushed the add-section-labels branch 2 times, most recently from 7b6f4ac to 268fc33 Compare July 11, 2024 12:31
@scharinger scharinger marked this pull request as ready for review July 11, 2024 12:33
@scharinger scharinger force-pushed the add-section-labels branch 3 times, most recently from 6e9b6d6 to 36f048f Compare July 11, 2024 12:47
Copy link
Contributor

@trew trew left a comment

Choose a reason for hiding this comment

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

Overall it looks nice. Some changes required though. I defer to other maintainers whether or not changes such as changeset.js/ts should be included or not. They are good changes but have nothing to do with the PR.

@scharinger scharinger force-pushed the add-section-labels branch 2 times, most recently from 1a73dbe to 2271f65 Compare August 14, 2024 07:52
@scharinger scharinger force-pushed the add-section-labels branch 4 times, most recently from bbbd2e2 to 77f3f24 Compare September 7, 2024 13:01
@scharinger scharinger requested review from trew and axis-d0op September 7, 2024 13:01
@trew
Copy link
Contributor

trew commented Sep 9, 2024

The range slider doesn't seems to work that well with sections. The track goes out of bounds. Also the mark label highlighting doesn't work properly when multiple marks have values that equal them.

@scharinger scharinger force-pushed the add-section-labels branch 2 times, most recently from 061bd41 to 3309ca6 Compare September 11, 2024 13:52
@scharinger
Copy link
Contributor Author

scharinger commented Sep 11, 2024

The range slider doesn't seems to work that well with sections. The track goes out of bounds. Also the mark label highlighting doesn't work properly when multiple marks have values that equal them.

Updated the gradient color logic to work for the range slider also.

image

Now also upgraded the logic on the section label active state. Only problem is that the thumb color on range sliders act oddly. But it's hard to fix currently as there is no support to handle style props on individual thumbs easily. We currently don't have a use case for range sliders using sections so maybe it could wait until we need it?

image

@trew
Copy link
Contributor

trew commented Sep 12, 2024

Only problem is that the thumb color on range sliders act oddly. ... We currently don't have a use case for range sliders using sections so maybe it could wait until we need it?

Yes. That one is ok for me to skip with this PR.

One final thing I noticed behavior-wise is that the active state for mark labels (when activeEqual is true) only works for the max value. That should probably test for all values.

Only "Boiling" is active. "Freezing" should be as well.
image

@scharinger scharinger force-pushed the add-section-labels branch 3 times, most recently from 19e8a1d to b90ee5e Compare September 12, 2024 08:19
@scharinger
Copy link
Contributor Author

Only problem is that the thumb color on range sliders act oddly. ... We currently don't have a use case for range sliders using sections so maybe it could wait until we need it?

Yes. That one is ok for me to skip with this PR.

One final thing I noticed behavior-wise is that the active state for mark labels (when activeEqual is true) only works for the max value. That should probably test for all values.

Only "Boiling" is active. "Freezing" should be as well. image

Updated the that the lower range value also is set as active.

image

Copy link
Contributor

@trew trew left a comment

Choose a reason for hiding this comment

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

Only 2 minor things left!

@trew
Copy link
Contributor

trew commented Sep 13, 2024

@axis-d0op The unresolved conversations blocks merging. How do I resolve them?

@axis-d0op
Copy link
Contributor

@scharinger så, nu ska alla kommentarer vara resolved. du behöver synca in main och pusha, samt se till att alla kommits är signerade. (signed commits).

@scharinger scharinger force-pushed the add-section-labels branch 3 times, most recently from e70e696 to 8092271 Compare September 16, 2024 18:21
- add sections defined a range with edges
- add section labels
- add section labels padding-bottom
- add section track colors
- make slider slider label be active
- add option so mark is only active when equal to slider value
@scharinger
Copy link
Contributor Author

Rebased.

Copy link
Contributor

@axis-d0op axis-d0op left a comment

Choose a reason for hiding this comment

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

LGTM / samuel & jimmy

@steabert steabert merged commit 93e1a39 into AxisCommunications:main Sep 17, 2024
4 checks passed
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.

4 participants