Skip to content
This repository has been archived by the owner on Sep 5, 2024. It is now read-only.

feat(slider): vertical slider, UI fixes and bug fixes #6538

Closed
wants to merge 1 commit into from
Closed

feat(slider): vertical slider, UI fixes and bug fixes #6538

wants to merge 1 commit into from

Conversation

EladBezalel
Copy link
Member

  • Added vertical slider
  • Improved UI
  • Fixed dynamic min/max with values not in range
  • Added round attribute to set how many numbers should be after the dot, default is 3, maximum is 6 to prevent scientific notation.
  • Made input width grow or shrink according the text length
  • Added discrete readonly mode
  • Added disabled on slider container

fixes #4371, #3259, #2421, #1221, #4576, #6996, #7093, #7093
closes #5874, #5872, #6051

@EladBezalel EladBezalel added the needs: review This PR is waiting on review from the team label Jan 4, 2016
@EladBezalel EladBezalel force-pushed the feat/slider-improvements branch from f9554e0 to 1e37d5d Compare January 10, 2016 22:59
@EladBezalel EladBezalel force-pushed the feat/slider-improvements branch 2 times, most recently from d2ce2fa to 7173881 Compare January 29, 2016 10:59
@ThomasBurleson ThomasBurleson added the in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs label Feb 1, 2016
topherfangio
topherfangio reviewed Feb 4, 2016
View reviewed changes
src/components/slider/slider.js
function SliderContainerDirective() {
return {
compile: function (elem) {
var slider = angular.element(elem[0].getElementsByTagName('md-slider'));
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe you can replace this with

var slider = elem.find('md-slider');

since you are finding an element, and not using a CSS query selector.

Copy link
Member Author

Choose a reason for hiding this comment

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

you're right

@topherfangio topherfangio added needs: work and removed needs: review This PR is waiting on review from the team labels Feb 4, 2016
@topherfangio
Copy link
Contributor

Aside from my line-comments above, I have seen a few visual issues when running the demos:

  1. On the Vertical Sliders demo, before interacting with them, a very tiny number id displayed inside the slider's circle. Seems a bit distracting, and it disappears after you focus/touch it.
  2. Upon focusing a slider via keyboard interaction, the slider is drastically larger than it should be (I think the bigger circle is simply 100% opacity instead of 38% or something)
  3. Focusing the Rating sliders via keyboard makes the circle appear and then disappear whereas clicking seems to have the proper functionality.

@EladBezalel EladBezalel force-pushed the feat/slider-improvements branch from 7173881 to 0727cfc Compare February 4, 2016 23:40
@EladBezalel
Copy link
Member Author

@topherfangio Talked to thomas about the

disaplay: flex,
justify-content: center

and he said it's ok but just add flex-direction: row

@EladBezalel EladBezalel force-pushed the feat/slider-improvements branch from 0727cfc to e04dd2c Compare February 5, 2016 00:21
@EladBezalel EladBezalel added needs: review This PR is waiting on review from the team and removed needs: work labels Feb 5, 2016
@ThomasBurleson ThomasBurleson added needs: work and removed needs: review This PR is waiting on review from the team labels Feb 6, 2016
@ThomasBurleson ThomasBurleson added this to the 1.0.6 milestone Feb 6, 2016
@EladBezalel EladBezalel force-pushed the feat/slider-improvements branch from e04dd2c to 81bae90 Compare February 6, 2016 16:53
@EladBezalel EladBezalel added needs: review This PR is waiting on review from the team and removed needs: work labels Feb 6, 2016
topherfangio
topherfangio reviewed Feb 26, 2016
View reviewed changes
src/components/slider/slider.js
@@ -280,8 +404,8 @@ function SliderDirective($$rAF, $window, $mdAria, $mdUtil, $mdConstant, $mdThemi
function stepValidator(value) {
if (angular.isNumber(value)) {
var formattedValue = (Math.round((value - min) / step) * step + min);
// Format to 3 digits after the decimal point - fixes #2015.
return (Math.round(formattedValue * 1000) / 1000);
// Format to 6 digits after the decimal point.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I've seen this value changed like 3 times now. Perhaps we should make it configurable?

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree

@EladBezalel EladBezalel force-pushed the feat/slider-improvements branch 4 times, most recently from 0f41386 to f1b7529 Compare February 26, 2016 21:15
@devversion devversion mentioned this pull request Feb 26, 2016
Closed
@topherfangio topherfangio modified the milestones: 1.1.0, 1.0.6 Feb 26, 2016
@topherfangio
Copy link
Contributor

I think this should be a 1.1.0 change as it modifies the structure of the the slider and makes a lot of changes that might potentially be breaking.

@EladBezalel
Copy link
Member Author

I don't think postponing this to 1.1.0 is smart, there're more additions than breaking changes.. I agree that this shouldn't be on the minor branch but on master for sure, more than that postponing it will make the rebasing even more hard for me.

@devversion devversion mentioned this pull request Feb 27, 2016
Closed
@EladBezalel EladBezalel force-pushed the feat/slider-improvements branch from f1b7529 to c176251 Compare February 27, 2016 23:39
@EladBezalel
feat(slider): vertical slider, UI fixes and bug fixes
914b35b 
- Added vertical slider
- Improved UI
- Fixed dynamic min/max with values not in range
- Added round attribute to set how many numbers should be after the dot, default is 3, maximum is 6 to prevent scientific notation.
- Made input width grow or shrink according the text length
- Added discrete readonly mode
- Added disabled on slider container

fixes #4371, #3259, #2421, #1221, #4576, #6996, #7093, #7093
closes #5874, #5872, #6051
@EladBezalel EladBezalel force-pushed the feat/slider-improvements branch from c176251 to 914b35b Compare February 27, 2016 23:42
@topherfangio
Copy link
Contributor

@elad master is 1.1.0 :-)

@EladBezalel
Copy link
Member Author

ah 👍 didn't realized it lol

@topherfangio is this still needs to be reviewed?

@topherfangio topherfangio added pr: merge ready This PR is ready for a caretaker to review and removed needs: review This PR is waiting on review from the team labels Feb 29, 2016
@topherfangio
Copy link
Contributor

@EladBezalel Just left one more comment re: the arrow keys, but I think that can be a separate discussion/PR if we want to do it later.

@ThomasBurleson ThomasBurleson added - Breaking Change and removed in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs labels Feb 29, 2016
@ThomasBurleson
Copy link
Contributor

Fixed with SHA e0abeb4

@EladBezalel EladBezalel deleted the feat/slider-improvements branch March 2, 2016 20:38
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees

@ThomasBurleson ThomasBurleson

Labels
- Breaking Change pr: merge ready This PR is ready for a caretaker to review
Projects
None yet
Milestone
1.1.0
Development

Successfully merging this pull request may close these issues.

md-slider rendering issue in the case of dynamic min/max
4 participants
@EladBezalel @topherfangio @ThomasBurleson @devversion