Skip to content

[Slider] Add new tick visibility modes #2897

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

Closed

Conversation

pubiqq
Copy link
Contributor

@pubiqq pubiqq commented Aug 8, 2022

Highlights

  • New tick visibility API: Slider.getTickVisibilityMode(), Slider.setTickVisibilityMode(int), and app:tickVisibilityMode.
  • New tick visibility modes:
    • visibleAll - All tick marks will be drawn, even if they are spaced too densely and overlap each other.
    • autoLimit - (Default) All tick marks will be drawn if they are not spaced too densely. Otherwise, the maximum allowed number of tick marks will be drawn.
    • autoHide - All tick marks will be drawn if they are not spaced too densely. Otherwise, the tick marks will not be drawn.
    • hidden - Tick marks will not be drawn at all.
  • Slider.setTickVisible(boolean) is deprecated in favor of Slider.setTickVisibilityMode(int).
  • app:tickVisible is deprecated in favor of app:tickVisibilityMode.

Migration

  • Slider.setTickVisible(true) -> Slider.setTickVisibilityMode(TICK_VISIBILITY_AUTO_LIMIT)
  • Slider.setTickVisible(false) -> Slider.setTickVisibilityMode(TICK_VISIBILITY_HIDDEN)
  • app:tickVisible="true" -> app:tickVisibilityMode="autoLimit"
  • app:tickVisible="false" -> app:tickVisibilityMode="hidden"

@pubiqq pubiqq force-pushed the slider/tick-visibility-mode branch 3 times, most recently from 143cf5b to d4290db Compare August 15, 2022 11:37
@pubiqq pubiqq requested a review from drchen August 15, 2022 11:47
@pubiqq pubiqq requested a review from drchen August 16, 2022 20:51
@pubiqq pubiqq force-pushed the slider/tick-visibility-mode branch from 803669f to 6e0c04b Compare August 29, 2022 12:32
Copy link
Contributor

@drchen drchen left a comment

Choose a reason for hiding this comment

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

Thank you so much for making the change!! Sorry that I have been pretty occupied recently and forgot about the change.

@drchen drchen added the Reviewing Internally An internal change has been created and sent for review. label Sep 29, 2022
@pubiqq
Copy link
Contributor Author

pubiqq commented Sep 29, 2022

No problem. Thanks a lot!

@drchen drchen self-assigned this Nov 17, 2022
@drchen
Copy link
Contributor

drchen commented Nov 17, 2022

Sorry for the delay. I'm working on this now. : )

@drchen
Copy link
Contributor

drchen commented Dec 12, 2022

I'm running into very weird issues when trying to merge the PR. Somehow the change blocks the argument validation from being done when running unit tests.

I don't really have time to debug it yet. If you can help check that on your end it will also be very helpful.

@pubiqq pubiqq force-pushed the slider/tick-visibility-mode branch from 6e0c04b to 76eed58 Compare December 13, 2022 16:42
@pubiqq
Copy link
Contributor Author

pubiqq commented Dec 13, 2022

Checked, fixed, and rebased.

@pubiqq pubiqq requested a review from drchen December 13, 2022 18:05
@pubiqq pubiqq force-pushed the slider/tick-visibility-mode branch from 76eed58 to c520d86 Compare February 13, 2023 15:25
@drchen
Copy link
Contributor

drchen commented Apr 27, 2023

Hey thanks for bumping this!! Sorry I missed your fix during my vacation.

Sending this for internal review now.

@drchen
Copy link
Contributor

drchen commented May 2, 2023

Hi, during the internal review, people raised that we are not sure what is the use scenario of autoHide mode?

Do you have an example of that?

@pubiqq
Copy link
Contributor Author

pubiqq commented May 2, 2023

Do you have an example of that?

#1233

As for me, I don't like the current default behavior either, because in the "dense" case ticks stop pointing to the position you can drag the slider to.

@pubiqq pubiqq force-pushed the slider/tick-visibility-mode branch from c520d86 to 86b3079 Compare August 24, 2023 01:56
@pubiqq pubiqq mentioned this pull request Oct 2, 2023
@pubiqq pubiqq force-pushed the slider/tick-visibility-mode branch from 86b3079 to b28f02e Compare December 7, 2023 18:35
@pubiqq pubiqq force-pushed the slider/tick-visibility-mode branch from b28f02e to 5984d88 Compare January 15, 2024 06:31
@pubiqq pubiqq force-pushed the slider/tick-visibility-mode branch from 5984d88 to 9059210 Compare February 14, 2024 20:54
@paulfthomas paulfthomas self-requested a review March 5, 2024 20:40
@drchen drchen assigned dsn5ft and unassigned drchen Mar 14, 2024
@drchen
Copy link
Contributor

drchen commented Mar 14, 2024

Dan, can you provide some insights here? I think the latest conversation we had regarding this is we probably need designers' opinion. ^^;

@pubiqq
Copy link
Contributor Author

pubiqq commented Mar 14, 2024

Just in case, I want to make it clear that this PR doesn't change the default behavior of the slider, but only adds two new strategies - autoHide (reason #1, reason #2) and visibleAll (I know this approach is not recommended by M3 specs, it's added mostly for symmetry with hidden).

@pubiqq pubiqq force-pushed the slider/tick-visibility-mode branch from 9059210 to 6eab388 Compare March 15, 2024 18:12
@pubiqq pubiqq force-pushed the slider/tick-visibility-mode branch 2 times, most recently from 8785ab7 to a19c637 Compare April 2, 2024 15:35
@paulfthomas
Copy link
Member

@drchen any update on this? I talked with our designers and they are not keen on adding this change, especially visibleAll, maybe removing it would be better.

@pubiqq
Copy link
Contributor Author

pubiqq commented Apr 11, 2024

If you want, I can delete visibleAll. I already explained why it was added, but I don't need it either and I don't insist on keeping it.

However, the autoHide should remain, that's what this PR was created for (explanation). Moreover, I believe the new behavior should be made the default one because the current behavior is not just illogical and confusing but directly contradicts the description of stop indicators.

@pubiqq pubiqq force-pushed the slider/tick-visibility-mode branch from a19c637 to 051dfeb Compare August 13, 2024 11:51
@pubiqq pubiqq force-pushed the slider/tick-visibility-mode branch from 051dfeb to f3dd354 Compare November 21, 2024 22:38
@paulfthomas
Copy link
Member

Hey, if visibleAll is removed and the default remains as the current behavior I should be able to have this merged.

@pubiqq pubiqq force-pushed the slider/tick-visibility-mode branch from f3dd354 to 82e1130 Compare November 25, 2024 17:50
@pubiqq
Copy link
Contributor Author

pubiqq commented Nov 25, 2024

Ok, done.

P.S. It would also be great if someone would look at my other PRs as well. I realize that adding features is more fun than fixing bugs, refactoring, and reviewing other people's code, but I believe it still needs to be done.

@paulfthomas
Copy link
Member

I'll take a look and try to merge this PR very soon, I know the delay is frustrating but we have a lot of things to consider internally so it's not just clicking the merge button unfortunately...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Reviewing Internally An internal change has been created and sent for review.
Projects
None yet
4 participants