-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
Conversation
@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. |
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. |
@kborchers ping |
@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. |
I'm not a fan of the light/dark classes. |
Why exactly do we need those classes? Can we get away with a single gif? |
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. |
Well, requiring them to add the proper class in the markup would be a divergence from anything we've done for theming. |
Neutral gray sounds good to me, as long as that works with our default theme (smoothness). |
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. |
@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 |
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. |
@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? |
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 |
@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. |
@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? |
How will this look on top of an already striped bar like Dot Lov? |
Damn, forgot about themes with stripes. I would say on indeterminate PB, we just remove the image in the |
That sounds good to me. |
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. |
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 |
val = 0; | ||
} else if( val === 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.
spacing
Why are we converting from |
@kborchers can you refactor the code accordingly? Or do you need more input? |
@jzaefferer So should I just check for |
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 |
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. |
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; | ||
} |
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.
this.indeterminate = val === false;
Looks much better to me. Can still squash into a single commit. |
…gressbar. Fixes #7624 - Progressbar: Support value: false for indeterminate progressbar
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" ); |
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.
Where did this go in the new code?
Landed in d3bc471 |
Initially, these changes started here #619. I have moved those commits to this branch which will contain all changes for the 1.10 release.