Progressbar: Add ability to set value: false for an indeterminate progressbar. Fixes #7624 - Progressbar: Support value: false for indeterminate progressbar #619

Closed
wants to merge 5 commits into
from

Projects

None yet

4 participants

@kborchers
jQuery Foundation member

No description provided.

kborchers added some commits Mar 27, 2012
@kborchers kborchers Progressbar: Add ability to set value: false for an indeterminate pro…
…gressbar. Fixes #7624 - Progressbar: Support value: false for indeterminate progressbar
7b482b6
@kborchers kborchers Progressbar: Whitespace 562bfe6
@kborchers
jQuery Foundation member

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.

@rdworth

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.

@rdworth rdworth commented on an outdated diff Mar 27, 2012
ui/jquery.ui.progressbar.js
val = 0;
+ } else if( val === false ) {
+ val = 100;
@rdworth
rdworth Mar 27, 2012

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.

@rdworth rdworth commented on an outdated diff Mar 27, 2012
ui/jquery.ui.progressbar.js
@@ -35,7 +35,12 @@ $.widget( "ui.progressbar", {
this.valueDiv = $( "<div class='ui-progressbar-value ui-widget-header ui-corner-left'></div>" )
.appendTo( this.element );
- this.oldValue = this._value();
+ if ( this.options.value !== false ) {
+ this.oldValue = this._value();
+ } else {
+ this.oldValue = false;
+ this.valueDiv.addClass( "ui-progressbar-animated" );
@rdworth
rdworth Mar 27, 2012

This should be handled in _refreshValue, rather than duplicated in both _create and _setOption

@rdworth rdworth and 1 other commented on an outdated diff Mar 27, 2012
ui/jquery.ui.progressbar.js
@@ -62,6 +67,11 @@ $.widget( "ui.progressbar", {
_setOption: function( key, value ) {
if ( key === "value" ) {
this.options.value = value;
+ if ( value !== false ) {
+ this.valueDiv.removeClass( "ui-progressbar-animated" );
+ } else {
+ this.valueDiv.addClass( "ui-progressbar-animated" );
+ }
@rdworth
rdworth Mar 27, 2012

These 5 lines can be 1 with .toggleClass( className, switch )

@scottgonzalez
scottgonzalez Mar 27, 2012

Also, ui-progressbar-animated is a bad name as it's visually descriptive, not semantically descriptive; ui-progressbar-indeterminate would be much better.

@rdworth

The word 'animated' should be changed to 'indeterminate' throughout.

@rdworth

Thanks so much for tackling this @kborchers !

@kborchers
jQuery Foundation member

Thanks for all of the feedback @rdworth !

@kborchers
jQuery Foundation member

As for the ugly GIF, that's what I was going for was to include in the base theme not a yellow one. :)

@scottgonzalez scottgonzalez commented on an outdated diff Mar 27, 2012
ui/jquery.ui.progressbar.js
@@ -16,7 +16,7 @@
$.widget( "ui.progressbar", {
version: "@VERSION",
options: {
- value: 0,
+ value: false,
@scottgonzalez
scottgonzalez Mar 27, 2012

The default should probably stay 0.

kborchers added some commits Mar 29, 2012
@kborchers kborchers Progressbar: Rename indeterminate class, return default value to 0, u…
…se NaN for the indeterminate value, replace animated gif and DRY code by moving much of it to _refreshValue
522338f
@kborchers kborchers Progressbar: Prevent firing change event on create for both determina…
…te and indeterminate
abb4eb5
@kborchers
jQuery Foundation member

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.

@kborchers
jQuery Foundation member

And here is an updated demo http://jsfiddle.net/kborchers/ha93d/15/

@gnarf
jQuery Foundation member

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

@kborchers
jQuery Foundation member

@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?

@scottgonzalez
jQuery Foundation member

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?

@scottgonzalez
jQuery Foundation member

@scottjehl Thoughts on styling?

@scottgonzalez
jQuery Foundation member

I've started a thread on free-aria to ensure that we're doing the right thing.

@scottgonzalez scottgonzalez reopened this Mar 29, 2012
@kborchers
jQuery Foundation member

@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.

@scottgonzalez
jQuery Foundation member

@hanshillen will be testing various browsers/AT to confirm they behave as expected.

@kborchers
jQuery Foundation member

I changed this to use CSS animations where supported.

@scottgonzalez
jQuery Foundation member

@scottjehl I think you were traveling when I pinged you earlier. Can you respond to my earlier question about styling?

@scottgonzalez
jQuery Foundation member

From Doug Neiner:

I think a indeterminate bar that doesn't animate doesn't make too much sense.
The goal is to put people at ease, not make them freak out cause its not moving.

@kborchers
jQuery Foundation member

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.

@kborchers kborchers closed this Oct 8, 2012
@kborchers kborchers referenced this pull request Oct 8, 2012
Closed

Progressbar: Code Review #738

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment