After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 707565 - appDisplay: Rework indicators animation
appDisplay: Rework indicators animation
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
ui-review
Depends on:
Blocks:
 
 
Reported: 2013-09-05 13:38 UTC by Carlos Soriano
Modified: 2013-09-16 16:57 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
appDisplay: Rework indicators animation (2.43 KB, patch)
2013-09-05 13:38 UTC, Carlos Soriano
none Details | Review
appDisplay: Rework indicators animation (2.43 KB, patch)
2013-09-05 17:50 UTC, Carlos Soriano
reviewed Details | Review
appDisplay: Rework indicators animation (2.92 KB, patch)
2013-09-10 23:18 UTC, Carlos Soriano
none Details | Review
appDisplay: Rework indicators animation (2.92 KB, patch)
2013-09-11 07:51 UTC, Carlos Soriano
none Details | Review
appDisplay: Rework indicators animation (2.83 KB, patch)
2013-09-13 17:37 UTC, Carlos Soriano
committed Details | Review

Description Carlos Soriano 2013-09-05 13:38:02 UTC
Try to match mockup animation
Comment 1 Carlos Soriano 2013-09-05 13:38:04 UTC
Created attachment 254181 [details] [review]
appDisplay: Rework indicators animation

Previously the animation was not entirely according to the mockup.
Now we are closer to the mockup.
Comment 2 Carlos Soriano 2013-09-05 17:50:37 UTC
Created attachment 254200 [details] [review]
appDisplay: Rework indicators animation

Previously the animation was not entirely according to the mockup.
Now we are closer to the mockup.
Comment 4 Florian Müllner 2013-09-10 19:31:38 UTC
Review of attachment 254200 [details] [review]:

Not obviously wrong, but not a fan either - can't we achieve the desired animation by tweaking the existing parameters?

::: data/theme/gnome-shell.css
@@ +944,2 @@
 .page-indicator {
+    padding: 15px 20px;

This looks like it belongs in the "remove padding" patch from the other bug? It doesn't look related to animations at all ...

::: js/ui/appDisplay.js
@@ +239,3 @@
             Tweener.addTween(children[i],
                              { translation_x: 0,
+                               time: INDICATORS_BASE_TIME + INDICATORS_ANIMATION_DELAY * i,

Mmmh, linear animation time based on number of pages? So if there are nine pages, I have to wait a full second until I can switch to the last page?
Comment 5 Carlos Soriano 2013-09-10 22:45:49 UTC
(In reply to comment #4)
> Review of attachment 254200 [details] [review]:
> 
> Not obviously wrong, but not a fan either - can't we achieve the desired
> animation by tweaking the existing parameters?
> 
I don't think so, since we have to let each dot a different animation time to achieve that "curve". At least I tried with all the variation with the current implementation, and no, and it is logical that no, since each dot has the same animation time.
> ::: data/theme/gnome-shell.css
> @@ +944,2 @@
>  .page-indicator {
> +    padding: 15px 20px;
> 
> This looks like it belongs in the "remove padding" patch from the other bug? It
> doesn't look related to animations at all ...
> 
yeah, but it is. In fact, before I had that padding addition in the patch you propose, but, it belongs here, since it is part to achieve the animation like the mockup, if not the animation is too "quick" (and not because of the time but from the distance). Do you still want this in the other patch?
> ::: js/ui/appDisplay.js
> @@ +239,3 @@
>              Tweener.addTween(children[i],
>                               { translation_x: 0,
> +                               time: INDICATORS_BASE_TIME +
> INDICATORS_ANIMATION_DELAY * i,
> 
> Mmmh, linear animation time based on number of pages? So if there are nine
> pages, I have to wait a full second until I can switch to the last page?
True, fixed in the upcoming set with a maximum time value (0.75s, and it's seeing beautiful still with 10 pages);
Comment 6 Carlos Soriano 2013-09-10 23:18:57 UTC
Created attachment 254633 [details] [review]
appDisplay: Rework indicators animation

Previously the animation was not entirely according to the mockup.
Now we are closer to the mockup.
Comment 7 Carlos Soriano 2013-09-11 07:51:02 UTC
Created attachment 254646 [details] [review]
appDisplay: Rework indicators animation

Previously the animation was not entirely according to the mockup.
Now we are closer to the mockup.
Comment 8 Carlos Soriano 2013-09-13 17:37:29 UTC
Created attachment 254879 [details] [review]
appDisplay: Rework indicators animation

Previously the animation was not entirely according to the mockup.
Now we are closer to the mockup.
Comment 9 Jakub Steiner 2013-09-16 11:48:23 UTC
Review of attachment 254879 [details] [review]:

Can't speak for code, but the result looks good to me.
Comment 10 Florian Müllner 2013-09-16 14:44:48 UTC
Review of attachment 254879 [details] [review]:

OK then.

::: data/theme/gnome-shell.css
@@ +943,2 @@
 .page-indicator {
+    padding: 15px 20px;

I really dislike how the "correctness" of the animation depends on the padding :-(
Comment 11 Carlos Soriano 2013-09-16 16:40:44 UTC
(In reply to comment #10)
> Review of attachment 254879 [details] [review]:
> 
> OK then.
> 
> ::: data/theme/gnome-shell.css
> @@ +943,2 @@
>  .page-indicator {
> +    padding: 15px 20px;
> 
> I really dislike how the "correctness" of the animation depends on the padding
> :-(

(In reply to comment #10)
> Review of attachment 254879 [details] [review]:
> 
> OK then.
> 
> ::: data/theme/gnome-shell.css
> @@ +943,2 @@
>  .page-indicator {
> +    padding: 15px 20px;
> 
> I really dislike how the "correctness" of the animation depends on the padding
> :-(

Is not that bad if we let the padding as it is, but yeah, it is better with less padding, and when more padding is added, more ugly becomes (too much quick) so...lets push this at it is, and let people/designers try it day to day, and if they feel that the padding is too much little, then we will see what we can do =)
Comment 12 Carlos Soriano 2013-09-16 16:53:01 UTC
Attachment 254879 [details] pushed as 93d9c16 - appDisplay: Rework indicators animation
Comment 13 Carlos Soriano 2013-09-16 16:57:06 UTC
I put a comment in the commit message about the padding thing, just to make
clear that *we* are not entirely sure with that and maybe we have to change it.