Created attachment 166019 [details] [review]
[panel] Add a style class while the overview is active

Designers have asked for the possibility to style the panel
differently in the overview.

Nothing fancy here - add .panel-overview style class while the overview is visible and remove it when it's hidden.

Comment on attachment 166019 [details] [review]
[panel] Add a style class while the overview is active

>+        Main.overview.connect('showing', Lang.bind(this, function () {
>+            this.actor.add_style_class_name('panel-overview');

I'm not a CSS guru, but it seems to me that this should be a pseudo-class? (It seems similar to "hover" and "focus".)

>+        Main.overview.connect('hiding', Lang.bind(this, function () {

You probably don't want to connect to that pair of signals. If the special style should only be in effect when the overview is fully visible, then you want to connect to 'shown' and 'hiding'. If you want it to be in effect when the overview is zooming in/out as well, then you want 'showing' and 'hidden'. ('showing' and 'hiding' means that the special style would be in effect when zooming into the overview, but not when zooming out of it.)

(In reply to comment #2)
> I'm not a CSS guru, but it seems to me that this should be a pseudo-class? (It
> seems similar to "hover" and "focus".)

Uhm - not sure, I always thought of pseudo-classes as an expression of element state, not some external condition - but then note how that definition already doesn't match existing classes like :first-child/:last-child, so I insist on being less of a CSS guru than you :)

I'll update the patch accordingly.


> >+        Main.overview.connect('hiding', Lang.bind(this, function () {
> 
> You probably don't want to connect to that pair of signals. If the special
> style should only be in effect when the overview is fully visible, then you
> want to connect to 'shown' and 'hiding'. If you want it to be in effect when
> the overview is zooming in/out as well, then you want 'showing' and 'hidden'.
> ('showing' and 'hiding' means that the special style would be in effect when
> zooming into the overview, but not when zooming out of it.)

Right. I automatically assumed that it would use the transition-duration property to match the overview animation, in which case the transition-to-transition definition would be correct.

Maybe
 * :overview-active (from 'shown' to 'hiding')
 * :overview-transition (from 'showing' to 'shown'/ from 'hiding' to 'hidden')?

could cover both cases?

(In reply to comment #2)
> (From update of attachment 166019 [details] [review])
> >+        Main.overview.connect('showing', Lang.bind(this, function () {
> >+            this.actor.add_style_class_name('panel-overview');
> 
> I'm not a CSS guru, but it seems to me that this should be a pseudo-class? (It
> seems similar to "hover" and "focus".)

The CSS pseudo-classes are all built into the spec - it's not a free-wheeling extensible set. So, for web programming, stuff like this is definitely a class not a pseudo-class.

We have a lot more flexibility, but I've sort of thought of pseudo-classes as being used for things that are more or less built-in to the widget, not about what our application is doing.

(In reply to comment #4)
> We have a lot more flexibility, but I've sort of thought of pseudo-classes as
> being used for things that are more or less built-in to the widget, not about
> what our application is doing.

Hm... and my theory was that style class is for identifying "type" ("this is a menu", "this is a notification popup"), and pseudo classes are for identifying a particular state within that type.

see also bug 612605

(In reply to comment #5)
> (In reply to comment #4)
> > We have a lot more flexibility, but I've sort of thought of pseudo-classes as
> > being used for things that are more or less built-in to the widget, not about
> > what our application is doing.
> 
> Hm... and my theory was that style class is for identifying "type" ("this is a
> menu", "this is a notification popup"), and pseudo classes are for identifying
> a particular state within that type.

I suspect the main reason for the separation between pseudo-class and class in CSS is one of namespacing. If there was no separate syntax for pseudo-classes, then the set of pseudo-classes would be fixed for all time, because if a new version of CSS added new pseudo-classes, it could break existing web pages.

In this interpretation, :focus is a shorthand for '.org-w3c-focus'

I'm not sure the world breaks down into type and state cleanly enough to make that interpretation useful. Certainly a few of the standard pseudo-classes are only partly state (I'm thinking of :first-child.)

Review of attachment 166019 [details] [review]:

I think this is fine this way with the style class. 

('panel-overview' is just a bit odd to me - it sort of implies that it's the  overview inside the panel or something. I can't think of that much that is better though - one approach would be to make it simply .in-overview, so you can do .panel.in-overview - is that better?)

(In reply to comment #7)
> I think this is fine this way with the style class.

Hmmm, but what about Dan's comment about the signals used? It is pretty odd to include the transition to the overview, but not the transition from the overview (even if it's very convenient for transitions)

 
> one approach would be to make it simply .in-overview, so you
> can do .panel.in-overview - is that better?)

Yes.

Comment on attachment 166019 [details] [review]
[panel] Add a style class while the overview is active

(In reply to comment #8)
> (In reply to comment #7)
> > I think this is fine this way with the style class.
> 
> Hmmm, but what about Dan's comment about the signals used? It is pretty odd to
> include the transition to the overview, but not the transition from the
> overview (even if it's very convenient for transitions)

Ah, missed that part of the comment. Yeah, I agree with Dan 100% on that.

Created attachment 166985 [details] [review]
[panel] Add a style class while the overview is active

(In reply to comment #9)
> Ah, missed that part of the comment. Yeah, I agree with Dan 100% on that.

OK.

Attachment 166985 [details] pushed as c18ff91 - [panel] Add a style class while the overview is active