Progressbar: Code Review #738

Closed
wants to merge 6 commits into
from

Projects

None yet

5 participants

@kborchers
Member

Initially, these changes started here #619. I have moved those commits to this branch which will contain all changes for the 1.10 release.

@kborchers
Member

@scottgonzalez @jzaefferer I have updated to use CSS animations where supported then fall back to a gif. The demo isn't quite working right yet and the animation didn't work in Opera for some reason but I would appreciate feedback before I mess with it anymore.

@jzaefferer
Member

Why do we need to generate images with ThemeRoller when they're just overlays, and the background is still themerollered?

CSS animations seem pretty complicated compared to just using an animated gif.

Either way, let's get rid of that animation option.

@jzaefferer
Member

@kborchers ping

@kborchers
Member

@jzaefferer The last commit removed the animation option then used css animation with a JS fallback to animated gif after testing for support. We could go strictly animated gif overlay with a themerolled background and remove a lot of the complexity. I'm fine either way, just let me know.

The only issue with the animated gif overlay is that the animated bars aren't transparent so it's white bars over the background as opposed to the css animation which has some opacity on the bars which I think looks nicer but also introduces some inconsistency between browsers. Doing just the animated gif is probably the best option but I'll wait for any final thoughts.

@scottgonzalez
Member

I'm not a fan of the light/dark classes.

@jzaefferer
Member

Why exactly do we need those classes? Can we get away with a single gif?

@kborchers
Member

What if they style the progressbar with a dark border and light (white) background? Then you need dark. But then opposite would require light. I guess I could remove the opacity and just do some sort of grey that would work anywhere (except on grey) but I think the two versions is more flexible.

@scottgonzalez
Member

Well, requiring them to add the proper class in the markup would be a divergence from anything we've done for theming.

@jzaefferer
Member

Neutral gray sounds good to me, as long as that works with our default theme (smoothness).

@toddparker

So are you looking for a way to have an animated diagonal stripe for indeterminate that will work across all themes? One approach would be to use alternating stripes of white and black and set it at a very low opacity (20-30%) over the TR background swatch.

@kborchers
Member

@toddparker Good point. Though, I can see that work with the CSS method but the GIF fallback won't work, right? Since you can only have a single color with lower opacity in an animated GIF, correct? That's the only reason I made 2 GIFs

@toddparker

I was thinking we'd just have the background a tiled stripes of solid black and solid white overlaid over the full themed bar. By setting the opacity of the tiled stripe to something around 25%, you'd see some texture but mostly the themed bar. This will be a bit muddier than ideal, but it should work everywhere.

@kborchers
Member

@toddparker Hmm, I think people will have an issue with the indeterminate bar being "muddier" and not matching all of the other themed bits of their app. Any other ideas or am I being too picky?

@kborchers
Member

solid black/white over the bar with opacity will make the color not be the expected color from the theme since it will be "whiter" or "blacker" which is why i went with my solution. also, the grey i don't think will work with a grey theme. not enough contrast

@toddparker

@kborchers - as a designer, I agree. This will be muddier but if the goal is to not swap between dark and light, this is a safe bet. Thinking about this more, the white color is only there to provide contrast for extremely dark progress bar fills which may not be common. We could probably get away with alternating black and transparent stripes and tell people that this won't work on pure black bars. It's all a tradeoff, especially since you need to support old IE.

@kborchers
Member

@toddparker ok, thanks. i will make changes to go with just black/transparent as that seems like the "best" option right now and see how that goes. any objections @scottgonzalez or @jzaefferer?

@scottgonzalez
Member

How will this look on top of an already striped bar like Dot Lov?

@kborchers
Member

Damn, forgot about themes with stripes. I would say on indeterminate PB, we just remove the image in the background style but keep the color and then use the animated overlay.

@scottgonzalez
Member

That sounds good to me.

@kborchers
Member

Well, I merged master so that's a lot of commits. Hopefully that's ok. Let me know if i did something wrong or need to redo anything.

@jzaefferer
Member

Looking at the overall diff we can probably squash all of this into a single commit. I don't see a problem with that.

Code wise, my only concern is that the indeterminate status is rather implicit, in most cases tested by isNaN( value ). We could make that more explicit, maybe with a indeterminate method or property. Should do that at least in the _refreshValue method, which has five isNaN( value ) calls.

@scottgonzalez scottgonzalez commented on an outdated diff Nov 3, 2012
ui/jquery.ui.progressbar.js
val = 0;
+ } else if( val === false ) {
@scottgonzalez
Member

Why are we converting from false to NaN anyway?

@jzaefferer
Member

@kborchers can you refactor the code accordingly? Or do you need more input?

@kborchers
Member

@jzaefferer So should I just check for false as @scottgonzalez was saying or go with the indeterminate method you suggested? I remember being told by one of you to do the NaN checks instead of just false way back when I first made those changes.

@scottgonzalez
Member

It was @rdworth in #619 (comment) I don't feel like tracking down exactly what was going on in that code because it was a lot of stuff that shouldn't be happening. I really don't think we should return NaN from value().

@kborchers
Member

OK, I should be able to get to that refactor and squash tonight. Sorry been busy with prepping for a couple of conferences so have been slacking on actually writing code.

@kborchers
Member

OK, let me know what you think of those changes. Also, I rebased on the progressbar branch which got rid of all of those master commits which should be fine since they don't affect progressbar. Let me know if I should still merge master in.

@jzaefferer jzaefferer commented on an outdated diff Nov 7, 2012
ui/jquery.ui.progressbar.js
@@ -74,20 +73,30 @@ $.widget( "ui.progressbar", {
_value: function() {
var val = this.options.value;
+ if( val === false ) {
+ this.indeterminate = true;
+ } else {
+ this.indeterminate = false;
+ }
@jzaefferer
jzaefferer Nov 7, 2012 Member

this.indeterminate = val === false;

@jzaefferer
Member

Looks much better to me. Can still squash into a single commit.

@kborchers
Member

I would appreciate another review to make sure I didn't miss anything in that merge and then I think this can be squashed and merged.

@mikesherov mikesherov commented on an outdated diff Nov 20, 2012
ui/jquery.ui.progressbar.js
this._trigger( "change" );
}
- if ( this.options.value === this.options.max ) {
- this._trigger( "complete" );
@mikesherov
mikesherov Nov 20, 2012 Member

Where did this go in the new code?

@kborchers
Member

Landed in d3bc471

@kborchers kborchers closed this Nov 22, 2012
@scottgonzalez scottgonzalez deleted the progressbar branch Apr 16, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment