-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Progressbar: Add ability to set value: false for an indeterminate progressbar. Fixes #7624 - Progressbar: Support value: false for indeterminate progressbar #619
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
Conversation
…gressbar. Fixes #7624 - Progressbar: Support value: false for indeterminate progressbar
This is not complete but I would appreciate a review. I saw this was a blocker and it didn't seem like anyone was working on it. I have a demo of my changes here http://jsfiddle.net/kborchers/ha93d/. The animated gif is bad so that will need to be updated and I'm not sure how to handle the ARIA attributes for the indeterminate progressbar so any input on those would be greatly appreciated. |
No need for an ugly animated gif: http://jsfiddle.net/ha93d/11/ . As far as a demo is concerned anyway. Of course, the ugly one in your PR is suited (other than the ugliness) for being part of the base theme. |
val = 0; | ||
} else if( val === false ) { | ||
val = 100; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like maybe you were trying to normalize to the max? But you've got 100 hard-coded.
As false is the default, false is no longer in an invalid value, so when asking for the value, it should not be normalized to a number/the max. Or if it is, let's normalize it to NaN. No better indeterminate number after all.
The word 'animated' should be changed to 'indeterminate' throughout. |
Thanks so much for tackling this @kborchers ! |
Thanks for all of the feedback @rdworth ! |
As for the ugly GIF, that's what I was going for was to include in the base theme not a yellow one. :) |
@@ -16,7 +16,7 @@ | |||
$.widget( "ui.progressbar", { | |||
version: "@VERSION", | |||
options: { | |||
value: 0, | |||
value: false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default should probably stay 0.
…se NaN for the indeterminate value, replace animated gif and DRY code by moving much of it to _refreshValue
…te and indeterminate
OK, I added a couple more commits that should get this to a pretty good place. I do have the yellow image in the PR and that will need to be swapped with a better one for the base theme before merging this. I wasn't sure if I should spend the time making this gif or if someone else was able to knock it out really fast. I will do it if you want. |
And here is an updated demo http://jsfiddle.net/kborchers/ha93d/15/ |
Can the current themeroller even colorize animated gifs? We should make sure of that before we land right? Other than that, this looks pretty hot Kris |
@gnarf37 Not that I know of. I was thinking we provide at least the grey one with the base theme and possibly a colored version for each of our theme roller themes and if they want a different color they provide it themselves. Another issue with that gif is it doesn't repeat-y very well so if the progressbar is taller than the gif it looks bad so that will need to be taken into account when creating a gif as well. I almost wonder if we could do something with a static image and animate(). Thoughts? |
I don't think we can do that. Whatever we do needs to work with ThemeRoller. Perhaps we can just generate a static striped image (which we know TR can already do), and then use CSS animations in browsers that support it? Would a static image be really bad for indeterminate progressbars? |
@scottjehl Thoughts on styling? |
I've started a thread on free-aria to ensure that we're doing the right thing. |
@scottgonzalez I believe we are. Prior to my changes the ARIA stuff looked right and then based on this http://www.w3.org/TR/wai-aria/states_and_properties#aria-valuenow I removed the valuenow for indeterminate. So according to spec we are right but I would like to here how it is supported so thanks for starting that thread. As for the static image, I think a static image might be ok and then CSS animations for browsers that support could be fine. |
@hanshillen will be testing various browsers/AT to confirm they behave as expected. |
I changed this to use CSS animations where supported. |
…here supported and just a static image when not
@scottjehl I think you were traveling when I pinged you earlier. Can you respond to my earlier question about styling? |
From Doug Neiner: I think a indeterminate bar that doesn't animate doesn't make too much sense. |
Closing this PR to move changes to a progressbar branch which will contain all Progressbar changes for 1.10 and will have a new PR for comprehensive review. |
No description provided.