Skip to content

[Slider] setup/get active and inactive tick radius #2994

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
Closed
minwang1 wants to merge 1 commit into from
Closed

[Slider] setup/get active and inactive tick radius #2994

minwang1 wants to merge 1 commit into from

Conversation

@minwang1
Copy link
Contributor

@minwang1 minwang1 commented Sep 25, 2022 •
edited by pekingme
Loading

Link to GitHub issues it solves. closes #1920

pekingme
pekingme previously approved these changes Sep 26, 2022
View reviewed changes
Copy link
Contributor

@pekingme pekingme left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

lib/java/com/google/android/material/slider/res/values/dimens.xml Outdated
<dimen name="mtrl_slider_track_height">4dp</dimen>

<dimen name="mtrl_slider_tick_radius">1dp</dimen>
<dimen name="mtrl_slider_inactive_tick_radius">1dp</dimen>
Copy link
Contributor

@pekingme pekingme Sep 26, 2022

Choose a reason for hiding this comment

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

Since this is same as the active one, let's remove this and just keep mtrl_slider_tick_radius and use it for both activeTickRadius and inactiveTickRadius.

Copy link
Contributor

@pekingme pekingme Sep 26, 2022

Choose a reason for hiding this comment

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

Please remove mtrl_slider_inactive_tick_radius

lib/java/com/google/android/material/slider/res/values/attrs.xml Outdated
Comment on lines 72 to 75
<!-- The radius of the active tick. -->
<attr name="activeTickRadius" format="dimension" />
<!-- The radius of the inactive tick. -->
<attr name="inactiveTickRadius" format="dimension" />
Copy link
Contributor

@pekingme pekingme Sep 26, 2022

Choose a reason for hiding this comment

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

Remember to add this in the public.xml.

Copy link
Contributor

@pekingme pekingme Sep 26, 2022

Choose a reason for hiding this comment

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

Please rename these to tickRadiusActive and tickRadiusInactive. And move them right below the tickColorXXX attributes.

@pekingme pekingme dismissed their stale review September 26, 2022 22:47

Sorry. Undo approval, change to request changes.

pekingme
pekingme requested changes Sep 26, 2022
View reviewed changes
Copy link
Contributor

@pekingme pekingme left a comment

Choose a reason for hiding this comment

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

Approval with change requests.

lib/java/com/google/android/material/slider/res/values/attrs.xml Outdated
Comment on lines 72 to 75
<!-- The radius of the active tick. -->
<attr name="activeTickRadius" format="dimension" />
<!-- The radius of the inactive tick. -->
<attr name="inactiveTickRadius" format="dimension" />
Copy link
Contributor

@pekingme pekingme Sep 26, 2022

Choose a reason for hiding this comment

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

Please rename these to tickRadiusActive and tickRadiusInactive. And move them right below the tickColorXXX attributes.

lib/java/com/google/android/material/slider/res/values/dimens.xml Outdated
<dimen name="mtrl_slider_track_height">4dp</dimen>

<dimen name="mtrl_slider_tick_radius">1dp</dimen>
<dimen name="mtrl_slider_inactive_tick_radius">1dp</dimen>
Copy link
Contributor

@pekingme pekingme Sep 26, 2022

Choose a reason for hiding this comment

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

Please remove mtrl_slider_inactive_tick_radius

@minwang1
Copy link
Contributor Author

minwang1 commented Sep 28, 2022

@pekingme thanks, please review again.

pekingme
pekingme requested changes Sep 28, 2022
View reviewed changes
Copy link
Contributor

@pekingme pekingme left a comment

Choose a reason for hiding this comment

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

Thanks! Could you also add the new attributes in the doc?

https://github.com/material-components/material-components-android/blob/master/docs/components/Slider.md#tick-mark-attributes

@minwang1
Copy link
Contributor Author

minwang1 commented Sep 29, 2022

please check it out

pekingme
pekingme approved these changes Oct 4, 2022
View reviewed changes
Copy link
Contributor

@pekingme pekingme left a comment

Choose a reason for hiding this comment

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

Great! Will pull this for internal reviews.

@pekingme pekingme added Reviewing Internally An internal change has been created and sent for review. and removed Work in progress (WIP) PR labels Oct 4, 2022
@pekingme pekingme self-assigned this Oct 4, 2022
@pekingme pekingme added Resolving Internal Failures The changes in this PR cause internal test failures. Team is working on resolving them. Reviewing Internally An internal change has been created and sent for review. and removed Reviewing Internally An internal change has been created and sent for review. Resolving Internal Failures The changes in this PR cause internal test failures. Team is working on resolving them. labels Oct 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pekingme pekingme pekingme approved these changes

Assignees

@pekingme pekingme

Labels

Reviewing Internally An internal change has been created and sent for review.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Slider] Tick size changeable

2 participants

@minwang1 @pekingme