Skip to content

Progressbar: Code Review #738

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 6 commits into from
Closed

Progressbar: Code Review #738

wants to merge 6 commits into from

Conversation

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

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

@kborchers ping

@kborchers
Copy link
Member Author

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

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

@jzaefferer
Copy link
Member

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

@kborchers
Copy link
Member Author

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

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

@jzaefferer
Copy link
Member

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

@toddparker
Copy link

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

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

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

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

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

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

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

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

@kborchers
Copy link
Member Author

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

That sounds good to me.

@kborchers
Copy link
Member Author

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

val = 0;
} else if( val === false ) {
Copy link
Member

Choose a reason for hiding this comment

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

spacing

@scottgonzalez
Copy link
Member

Why are we converting from false to NaN anyway?

@jzaefferer
Copy link
Member

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

@kborchers
Copy link
Member Author

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

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

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.

this.indeterminate = true;
} else {
this.indeterminate = false;
}
Copy link
Member

Choose a reason for hiding this comment

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

this.indeterminate = val === false;

@jzaefferer
Copy link
Member

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

@kborchers
Copy link
Member Author

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.

this._trigger( "change" );
}
if ( this.options.value === this.options.max ) {
this._trigger( "complete" );
Copy link
Member

Choose a reason for hiding this comment

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

Where did this go in the new code?

@kborchers
Copy link
Member Author

Landed in d3bc471

@kborchers kborchers closed this Nov 22, 2012
@scottgonzalez scottgonzalez deleted the progressbar branch April 16, 2014 14:33
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.

5 participants