Implement indeterminate state for SC.ProgressView (fixes #821) #823

Closed
wants to merge 14 commits into
from

2 participants

@unicolet

This completes the implementation of SC.ProgressView for the Ace theme by adding the code for handling the isIndeterminate and isRunning states.

The pr includes unit tests which I have make sure they pass on FF, Chrome, Safari. Not tested on IE.

Caveats:

  • for the background I have reused (and cropped) the striped image from the legacy theme. I think it blends just fine, but someone might want to improve the graphics to achieve full compatibility

  • the same image causes a warning to be reported by abbot because it is quite wide (actually just 24px wide) and, once included, it forces abbot to generate a much wider sprite. Someone with better graphics skills than mine could redraw the bg and straighten the stripes so that they'll fit in 10px, which would make abbot happy again

@publickeating
SproutCore member

Hi,
Just wanted to let you know that I'm working on this one. A couple things: as you mentioned, the image used should be updated to better match the Ace theme and I actually found that such an image already exists in a PSD inside the project. So I cleaned that one up a bit and exported it and it's a simple change to use it. There are two other larger things though that need to be addressed.

The first is that the animation uses the Run Loop which will be detrimental to performance. I see that you used a very conservative animation frame-rate, but even still it won't be good to fire a run loop each time just to animate the bar. After some exploration, I found that by using requestAnimationFrame and by offloading the animation into its own code loop, we can do 60fps with very little power. The nice thing about requestAnimationFrame is that the browser will stop the animation running if the tab isn't visible which will save power. The change isn't a lot of code, but it took me a bit to really tune it up nicely.

The second is that I think we should have a CSS animation too and only use the JS as a fallback, which I think will be a pretty simple addition to the .is-running class. I haven't looked at that yet, but I would like that to be added at the same time. If you've got the bandwidth, please have a look.

So I've made most of the changes I mentioned and pushed it to https://github.com/sproutcore/sproutcore/tree/team/publickeating/indeterminate_progress_view . The outstanding work is: add CSS animation, fix the sprite so that it doesn't throw those warnings as you noted above and add whatever unit tests make sense.

Thanks.

@unicolet

I have given an in depth look at your changes and unfortunately I see the following problems with them:

  1. using the global selector SC.$('.content .middle') means that ANY progress view in the page will automatically be upgraded to indeterminate and animated. Luckily it was easy to fix (see my commits above)

1.1 related to the previous item: unit tests now fail because the global selector in the render delegate can't find the div to animate

  1. use of the deletegate attribute _offset causes the animation to jump if a page has more than one ProgressView: not fixed

  2. _offset also causes the animation to accelerate linearly with the number of ProgressView running on the same page: fixed

  3. I have created an image (in Gimp, as I don't have PS on this computer) that fits Abbot's 10px constraint. Honestly, it's hard to get something smooth in just 10px, but this one doesn't look so bad.

These are just preliminary thoughts/ideas that I wanted to share with you.
I'll be working on this a little more as I am thinking using a render queue in the delegate so that I can get unit tests to pass again (that's my primary concern right now).

@unicolet

Tyler, please take a look at my last push, now all tests pass.
Outstanding: css animations

@publickeating
SproutCore member

Thanks for the improvements. I found a couple minor leftovers. The main two things were that the track was being animated along with the bar, because they both matched .middle and toggling isIndeterminate didn't clean up the background image position. I've fixed all that, it was just some minor tweaks, and I also decided that the 42 pixel wide background has to be the proper background to use.

In order to use the 42px wide background, I simply included it with sc_static and didn't use slices at all. This means that the repeat-x sprite won't include it and Chance won't complain. It also means that that image will have to be loaded independently, but only if someone is using indeterminate Ace progress views. I think that it's a fair enough tradeoff, since the sprite will be used by all Ace users and it's better to keep it small as possible.

So I'm going to check on the CSS only solution and then hopefully commit what you've got into master later today.

Thanks for your help!

@unicolet

If you want I can take care of the CSS-only solution, I only need to know if there is a way in SC to detect browsers that don't support it.

@publickeating
SproutCore member
@publickeating
SproutCore member

Pushed this directly into master.

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