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

Added pinLabel property. #167

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

Inrego
Copy link

@Inrego Inrego commented Jan 19, 2017

Solves #133 although with a little more customization than suggested there.

I've added an optional pinLabel property that if set, will be used for the pin label instead of just the immediateValue. That way, you can create your own text with bindings, like so:

immediate-value="{{durationSliderValue}}" pin-label="[[_readableDuration(durationSliderValue)]]"

If not set, the value will be used as normal, so no breaking changes.

Copy link
Contributor

@keanulee keanulee left a comment

Choose a reason for hiding this comment

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

So this doesn't really address #133 since it just replaces the label (instead of appending/prepending). Is this PR something that you actually want?

pinLabel: {
type: String,
value: "",
notify: false
Copy link
Contributor

Choose a reason for hiding this comment

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

Default value and notify property are unnecessary.

Copy link
Author

Choose a reason for hiding this comment

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

I've removed them now.

@Inrego
Copy link
Author

Inrego commented Jan 19, 2017

Well it does offer a solution to that issue. I implemented this because I had a problem where I needed to format the numbers into a timestamp instead of showing just a number, since I use this slider as a slider to seek in a video. This offers a way to support both.
The text does get a little cramped in there, though so people will have to keep that in mind when making labels, that they should be rather short. Unless someone can make the pin dynamic size or something like that.

* number. Use if you need to format the label in any way.
*/
pinLabel: {
type: String
Copy link
Contributor

Choose a reason for hiding this comment

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

My mistake - you do need a default value of '' for pinLabel because the value$="[[_getPinLabel(pinLabel, immediateValue)]]" computed binding won't be evaluated unless both arguments are defined.

@@ -1,6 +1,6 @@
{
"name": "paper-slider",
"version": "1.0.12",
"version": "1.0.13",
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't update bower.json/version number with this PR - we will do that separately and publish a release separately.

@keanulee
Copy link
Contributor

Since this is a new feature, tt'd be great to add a demo and a test.

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

Successfully merging this pull request may close these issues.

3 participants