Skip to content

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

Closed
wants to merge 5 commits into from

Conversation

kborchers
Copy link
Member

No description provided.

…gressbar. Fixes #7624 - Progressbar: Support value: false for indeterminate progressbar
@kborchers
Copy link
Member Author

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
Copy link
Contributor

rdworth commented Mar 27, 2012

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;
Copy link
Contributor

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.

@rdworth
Copy link
Contributor

rdworth commented Mar 27, 2012

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

@rdworth
Copy link
Contributor

rdworth commented Mar 27, 2012

Thanks so much for tackling this @kborchers !

@kborchers
Copy link
Member Author

Thanks for all of the feedback @rdworth !

@kborchers
Copy link
Member Author

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,
Copy link
Member

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
@kborchers
Copy link
Member Author

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
Copy link
Member Author

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

@gnarf
Copy link
Member

gnarf commented Mar 29, 2012

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
Copy link
Member Author

@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
Copy link
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
Copy link
Member

@scottjehl Thoughts on styling?

@scottgonzalez
Copy link
Member

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

@kborchers
Copy link
Member Author

@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
Copy link
Member

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

@kborchers
Copy link
Member Author

I changed this to use CSS animations where supported.

…here supported and just a static image when not
@scottgonzalez
Copy link
Member

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

@scottgonzalez
Copy link
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
Copy link
Member Author

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants