Skip to content

feat: Add selectionBarGradient option to customize the selection bar #473

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

Merged
merged 3 commits into from
Dec 12, 2016

Conversation

Masadow
Copy link
Contributor

@Masadow Masadow commented Dec 7, 2016

Add selectionBarGradient option which create a linear gradient for the selection bar. The option

accepts an object that contains from and to properties which are colors describing the gradient

Here's a fiddle demonstrating the feature: http://jsfiddle.net/masadow/cqk61f0a/

Additional note: There's room for improvement, for example, the ticks are not affected by the gradient. I think it's another feature but if someone feels like it's a good idea, feel free to give it a go.

@codecov-io
Copy link

codecov-io commented Dec 7, 2016

Current coverage is 100% (diff: 100%)

Merging #473 into master will not change coverage

@@           master   #473   diff @@
====================================
  Files           1      1          
  Lines         921    929     +8   
  Methods         0      0          
  Messages        0      0          
  Branches        0      0          
====================================
+ Hits          921    929     +8   
  Misses          0      0          
  Partials        0      0          

Powered by Codecov. Last update 36a012c...46cca0a

@ValentinH
Copy link
Member

Why not just doing it via CSS?

@Masadow
Copy link
Contributor Author

Masadow commented Dec 8, 2016

It's not possible with the range slider. Or I don't see how without changing the whole DOM.

Btw, it seems like a test must be written to hit 100% of code coverage ?

@@ -1268,6 +1269,20 @@
this.scope.barStyle = {
backgroundColor: color
};
} else if (this.options.selectionBarGradient) {
var offset = this.options.showSelectionBarFromValue !== null ? this.valueToPosition(this.options.showSelectionBarFromValue) : 0,
reversed = offset - position > 0 ^ isSelectionBarFromRight,
Copy link
Member

Choose a reason for hiding this comment

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

What does the "^" sign?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a XOR. It means either offset - position > 0 or isSelectionBarFromRight is true, but not both.

@ValentinH
Copy link
Member

Ok I understand now. Thanks for this pretty complete pull request. Indeed, you need to add tests to cover your changes. You can start from an existing one related to the selection bar. ;)

@Masadow
Copy link
Contributor Author

Masadow commented Dec 8, 2016

I'll write some test within the day and update my PR. Can I add another commit or should I git reset --soft HEAD~1 my current commit and create a new one ?

@ValentinH
Copy link
Member

Adding another commit is OK ;)

} else if (this.options.selectionBarGradient) {
var offset = this.options.showSelectionBarFromValue !== null ? this.valueToPosition(this.options.showSelectionBarFromValue) : 0,
reversed = offset - position > 0 ^ isSelectionBarFromRight,
direction = this.options.vertical ? 'top' : (reversed ? 'left' : 'right');
Copy link
Member

Choose a reason for hiding this comment

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

the vertical + reversed case is not handled I think. Look this demo: http://jsfiddle.net/nLk7p00v/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, I did not see pay attention to that option showSelectionBarEnd. It's fixed by now

Copy link
Member

Choose a reason for hiding this comment

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

I'm on the phone so I might be wrong but the vertical example in the fiddle still seems broken?

@ValentinH
Copy link
Member

The vertical reversed example is still broken:
image

There is a white part on top...

@Masadow
Copy link
Contributor Author

Masadow commented Dec 12, 2016

I made a mistake in the fiddle. I was not pointing to the right branch, please try with <script src="https://cdn.rawgit.com/Masadow/angularjs-slider/gradient-selection-bar/dist/rzslider.min.js"></script>

@ValentinH
Copy link
Member

OK you are right. I think it is OK to merge it, I'll just need you to rebase on top of master to resolve the current conflicts (just the dist files).

For later, I just point out that the showSelectionBarFromValue doesn't work with the gradient but it'll be more complex to handle it I think: http://jsfiddle.net/o0x97L60/

Add selectionBarGradient option which create a linear gradient for the selection bar. The option

accepts an object that contains `from` and `to` properties which are colors describing the gradient
@Masadow Masadow force-pushed the gradient-selection-bar branch from 9553442 to 46cca0a Compare December 12, 2016 14:15
@Masadow
Copy link
Contributor Author

Masadow commented Dec 12, 2016

That's fine for conflicts, I rebased on master already.

I agree about showSelectionBarFromValue, I partially handled it but I did not calculate the length of the background gradient. I thought I would not try to handle it since there are multiple design decisions to go from

@ValentinH
Copy link
Member

Indeed, I don't even know how I would handle it, so that's fine for now.

@ValentinH ValentinH merged commit b84e866 into angular-slider:master Dec 12, 2016
@ValentinH
Copy link
Member

Thanks for your contribution! 👍

@ValentinH
Copy link
Member

Released via 5.9.0

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.

3 participants