Skip to content
This repository

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Progressbar: Code Review #738

Closed
wants to merge 6 commits into from

5 participants

Kris Borchers Jörn Zaefferer Scott González Todd Parker Mike Sherov
Kris Borchers
Owner

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

Kris Borchers
Owner

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

Jörn Zaefferer
Owner

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.

Jörn Zaefferer
Owner

@kborchers ping

Kris Borchers
Owner

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

Scott González

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

Jörn Zaefferer
Owner

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

Kris Borchers
Owner

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.

Scott González

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

Jörn Zaefferer
Owner

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

Todd Parker

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.

Kris Borchers
Owner

@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

Todd Parker

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.

Kris Borchers
Owner

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

Kris Borchers
Owner

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

Todd Parker

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

Kris Borchers
Owner

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

Scott González

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

Kris Borchers
Owner

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.

Scott González

That sounds good to me.

Kris Borchers
Owner

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.

Jörn Zaefferer
Owner

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.

ui/jquery.ui.progressbar.js
((6 lines not shown))
79 79 val = 0;
  80 + } else if( val === false ) {
1
Scott González Owner

spacing

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

Why are we converting from false to NaN anyway?

Jörn Zaefferer
Owner

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

Kris Borchers
Owner

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

Scott González

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().

Kris Borchers
Owner

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.

Kris Borchers
Owner

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.

ui/jquery.ui.progressbar.js
... ... @@ -74,20 +73,30 @@ $.widget( "ui.progressbar", {
74 73
75 74 _value: function() {
76 75 var val = this.options.value;
  76 + if( val === false ) {
  77 + this.indeterminate = true;
  78 + } else {
  79 + this.indeterminate = false;
  80 + }
1
Jörn Zaefferer Owner

this.indeterminate = val === false;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Jörn Zaefferer
Owner

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

Kris Borchers
Owner

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.

ui/jquery.ui.progressbar.js
((21 lines not shown))
117 125 this._trigger( "change" );
118 126 }
119   - if ( this.options.value === this.options.max ) {
120   - this._trigger( "complete" );
1
Mike Sherov Collaborator

Where did this go in the new code?

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

Landed in d3bc471

Scott González scottgonzalez deleted the branch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
9 demos/progressbar/animated.html
@@ -9,14 +9,11 @@
9 9 <script src="../../ui/jquery.ui.widget.js"></script>
10 10 <script src="../../ui/jquery.ui.progressbar.js"></script>
11 11 <link rel="stylesheet" href="../demos.css">
12   - <style>
13   - .ui-progressbar .ui-progressbar-value { background-image: url(images/pbar-ani.gif); }
14   - </style>
15 12 <script>
16 13 $(function() {
17 14 $( "#progressbar" ).progressbar({
18 15 value: 59
19   - });
  16 + }).find( ".ui-progressbar-value div" ).addClass( "ui-progressbar-overlay" );
20 17 });
21 18 </script>
22 19 </head>
@@ -27,10 +24,10 @@
27 24 <div class="demo-description">
28 25 <p>
29 26 This progressbar has an animated fill by setting the
30   -<code>background-image</code>
  27 +<code>ui-progressbar-overlay</code> class
31 28 on the
32 29 <code>.ui-progressbar-value</code>
33   -element, using css.
  30 +element's overlay div.
34 31 </p>
35 32 </div>
36 33 </body>
53 demos/progressbar/indeterminate.html
... ... @@ -0,0 +1,53 @@
  1 +<!doctype html>
  2 +<html lang="en">
  3 +<head>
  4 + <meta charset="utf-8">
  5 + <title>jQuery UI Progressbar - Indeterminate Value</title>
  6 + <link rel="stylesheet" href="../../themes/base/jquery.ui.all.css">
  7 + <script src="../../jquery-1.8.3.js"></script>
  8 + <script src="../../ui/jquery.ui.core.js"></script>
  9 + <script src="../../ui/jquery.ui.widget.js"></script>
  10 + <script src="../../ui/jquery.ui.progressbar.js"></script>
  11 + <link rel="stylesheet" href="../demos.css">
  12 + <script>
  13 + $(function() {
  14 + $( "#progressbar" ).progressbar({
  15 + value: false
  16 + });
  17 + $( "button" ).on( "click", function( event ) {
  18 + var target = $( event.target ),
  19 + pbar = $( "#progressbar" ),
  20 + pbarValue = pbar.find( ".ui-progressbar-value" );
  21 +
  22 + if ( target.is( "#numButton" ) ) {
  23 + pbar.progressbar( "option", {
  24 + value: Math.floor( Math.random() * 100 )
  25 + });
  26 + } else if ( target.is( "#colorButton" ) ) {
  27 + pbarValue.css({
  28 + "background": '#' + Math.floor( Math.random() * 16777215 ).toString( 16 )
  29 + });
  30 + } else if ( target.is( "#falseButton" ) ) {
  31 + pbar.progressbar( "option", "value", false );
  32 + }
  33 + });
  34 + });
  35 + </script>
  36 + <style>
  37 + #progressbar .ui-progressbar-value {
  38 + background-color: #CCCCCC;
  39 + }
  40 + </style>
  41 +</head>
  42 +<body>
  43 +
  44 +<div id="progressbar"></div>
  45 +<button id="numButton">Random Value - Determinate</button>
  46 +<button id="falseButton">Indeterminate</button>
  47 +<button id="colorButton">Random Color</button>
  48 +
  49 +<div class="demo-description">
  50 +<p>Indeterminate progress bar and switching between determinate and indeterminate styles.</p>
  51 +</div>
  52 +</body>
  53 +</html>
1  demos/progressbar/index.html
@@ -10,6 +10,7 @@
10 10 <li><a href="default.html">Default functionality</a></li>
11 11 <li><a href="animated.html">Animated</a></li>
12 12 <li><a href="resize.html">Resizable progressbar</a></li>
  13 + <li><a href="indeterminate.html">Indeterminate</a></li>
13 14 </ul>
14 15
15 16 </body>
6 tests/unit/progressbar/progressbar_events.js
@@ -23,7 +23,7 @@ test( "change", function() {
23 23 });
24 24
25 25 test( "complete", function() {
26   - expect( 3 );
  26 + expect( 4 );
27 27 var value,
28 28 changes = 0,
29 29 element = $( "#progressbar" ).progressbar({
@@ -32,12 +32,14 @@ test( "complete", function() {
32 32 deepEqual( element.progressbar( "value" ), value, "change at " + value );
33 33 },
34 34 complete: function() {
35   - equal( changes, 2, "complete triggered after change" );
  35 + equal( changes, 3, "complete triggered after change and not on indeterminate" );
36 36 }
37 37 });
38 38
39 39 value = 5;
40 40 element.progressbar( "value", value );
  41 + value = false;
  42 + element.progressbar( "value", value );
41 43 value = 100;
42 44 element.progressbar( "value", value );
43 45 });
BIN  themes/base/images/animated-overlay.gif
11 themes/base/jquery.ui.progressbar.css
@@ -9,4 +9,13 @@
9 9 * http://docs.jquery.com/UI/Progressbar#theming
10 10 */
11 11 .ui-progressbar { height:2em; text-align: left; overflow: hidden; }
12   -.ui-progressbar .ui-progressbar-value {margin: -1px; height:100%; }
  12 +.ui-progressbar .ui-progressbar-value { margin: -1px; height:100%; }
  13 +.ui-progressbar .ui-progressbar-value .ui-progressbar-overlay {
  14 + background: url("images/animated-overlay.gif");
  15 + height: 100%;
  16 + filter: alpha(opacity=25);
  17 + opacity: 0.25;
  18 +}
  19 +.ui-progressbar .ui-progressbar-indeterminate {
  20 + background-image: none;
  21 +}
35 ui/jquery.ui.progressbar.js
@@ -36,7 +36,7 @@ $.widget( "ui.progressbar", {
36 36 "aria-valuenow": this.options.value
37 37 });
38 38
39   - this.valueDiv = $( "<div class='ui-progressbar-value ui-widget-header ui-corner-left'></div>" )
  39 + this.valueDiv = $( "<div class='ui-progressbar-value ui-widget-header ui-corner-left'><div></div></div>" )
40 40 .appendTo( this.element );
41 41
42 42 this.oldValue = this.options.value;
@@ -71,16 +71,19 @@ $.widget( "ui.progressbar", {
71 71 val = newValue;
72 72 }
73 73
  74 + this.indeterminate = val === false;
  75 +
74 76 // sanitize value
75 77 if ( typeof val !== "number" ) {
76 78 val = 0;
77 79 }
78   - return Math.min( this.options.max, Math.max( this.min, val ) );
  80 + return this.indeterminate ? false : Math.min( this.options.max, Math.max( this.min, val ) );
79 81 },
80 82
81 83 _setOptions: function( options ) {
82 84 var val = options.value;
83 85
  86 + // Ensure "value" option is set after other values (like max)
84 87 delete options.value;
85 88 this._super( options );
86 89
@@ -106,26 +109,36 @@ $.widget( "ui.progressbar", {
106 109 },
107 110
108 111 _percentage: function() {
109   - return 100 * this.options.value / this.options.max;
  112 + return this.indeterminate ? 100 : 100 * this.options.value / this.options.max;
110 113 },
111 114
112 115 _refreshValue: function() {
113   - var percentage = this._percentage();
  116 + var value = this.options.value,
  117 + percentage = this._percentage(),
  118 + overlay = this.valueDiv.children().eq( 0 );
  119 +
  120 + overlay.toggleClass( "ui-progressbar-overlay", this.indeterminate );
  121 + this.valueDiv.toggleClass( "ui-progressbar-indeterminate", this.indeterminate );
114 122
115   - if ( this.oldValue !== this.options.value ) {
116   - this.oldValue = this.options.value;
  123 + if ( this.oldValue !== value ) {
  124 + this.oldValue = value;
117 125 this._trigger( "change" );
118 126 }
119   - if ( this.options.value === this.options.max ) {
  127 + if ( value === this.options.max ) {
120 128 this._trigger( "complete" );
121 129 }
122 130
123 131 this.valueDiv
124   - .toggle( this.options.value > this.min )
125   - .toggleClass( "ui-corner-right", this.options.value === this.options.max )
  132 + .toggle( this.indeterminate || value > this.min )
  133 + .toggleClass( "ui-corner-right", value === this.options.max )
126 134 .width( percentage.toFixed(0) + "%" );
127   - this.element.attr( "aria-valuemax", this.options.max );
128   - this.element.attr( "aria-valuenow", this.options.value );
  135 + if ( this.indeterminate ) {
  136 + this.element.removeAttr( "aria-valuemax" );
  137 + this.element.removeAttr( "aria-valuenow" );
  138 + } else {
  139 + this.element.attr( "aria-valuemax", this.options.max );
  140 + this.element.attr( "aria-valuenow", value );
  141 + }
129 142 }
130 143 });
131 144

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.