Created attachment 312855 [details] [review]
Possible Patch for Header Highlight When Sensitive

add class to room-list-header on connect error; add css for connect-error class on hover

Created attachment 312859 [details] [review]
Simplified previous patch, fixed differing margins at end of headers

fmuellner made me aware of not actually needing to add a class, so I fixed that. Also let me know about how to fix the header margins, so I bundled it in.

Review of attachment 312859 [details] [review]:

The change looks mostly good to me, though I'll leave the final judgment on the details to Bastian.

Regarding the commit message, it should try to follow this template:

"optionalPrefix: Concisely describe what the patch does

Outline briefly why the change is done, and if necessary explain aspects of the
change that are not obvious from the code."


So in this case something like:
"ui: Highlight header on hover if sensitive

In case of account errors, room headers become clickable, but we currently 
don't convey that to the user. So highlight sensitive headers on hovers
to make this more obvious."

::: data/resources/application.css
@@ +47,3 @@
 }
 
+.polari-room-list .room-list-header:hover {

Does it make sense to add
 ".room-list-header:checked"
as well to use the same highlight while the popover is shown?

::: data/resources/room-list-header.ui
@@ -3,3 @@
   <template class="Gjs_RoomListHeader" parent="GtkMenuButton">
-    <property name="popover">errorPopover</property>
-    <property name="margin-bottom">4</property>

Not sure about moving the bottom padding - it moves the whitespace from below the border above, which disconnects the header text from its underline.

Review of attachment 312859 [details] [review]:

Nice work! I have a few visual corrections:

1) it would be better to reuse the same color as the other rows. if you add the style classes "activatable" and "list-row" to the roomlistheader object you should be able to achieve that (you can also try doing this from GTK+ Inspector).
2) I dont think there's a need to change the padding as you did in the patch. Also because it changes the way the underline is rendered.

That should give you a result more closely resembling the screenshot I attached.

Okay, cool. I'll post a new patch with the above in mind. :)

Created attachment 313002 [details] [review]
ui: Highlight header on hover if sensitive

If there is a connection error, there is no user input.
Highlight headers upon hovering, as is done with room rows,
to indicate they are clickable.

Review of attachment 313002 [details] [review]:

Two more nits :-)

> diff --git a/src/lib/.dirstamp b/src/lib/.dirstamp
> new file mode 100644
> index 0000000..e69de29
This file should not be included in the patch.

> If there is a connection error, there is no user input.
I dont make much out of this sentence, please see Florian's suggestion in Comment 3.

Ha, I didn't even notice that file. Whoops.

Yeah, I should probably not write commit messages at early hours :)

Created attachment 313016 [details] [review]
ui: Highlight header on hover if sensitive

In case of account errors, room headers become clickable, but we currently 
don't convey that to the user. So highlight sensitive headers on hovers
to make this more obvious.

Review of attachment 313016 [details] [review]:

See comment below - also the commit message shown in comment #9 is missing from the actual patch.

Bastian, do we want to add a style when the popover is up as well (i.e. :checked)?

::: src/roomList.js
@@ +202,3 @@
             child = 'error';
+            this.get_style_context().add_class('list-row');
+            this.get_style_context().add_class('activatable');

Adding this each time there is an error is ugly, even though GTK+ handles this and doesn't add a duplicated style class. As a rule of thumb, every conditional add_class() should be matched by a remove_class(), and where removing a class doesn't make sense (as in this case), it should be added unconditionally - the template file[0] is a good place for that.

[0] https://git.gnome.org/browse/polari/tree/data/resources/room-list-header.ui#n10

Yeah, that's a fair point - but where would one add that class? Looking at the GtkBuilder docs (I assume that's what I'm supposed to be looking at), I appear to be confusing myself a bit. Does Polari define custom tags such as <packing>? Or, are you implying you wish to add more class names in the initial <style> tag?

(In reply to Cody Welsh from comment #11)
> Or, are you implying you wish to add more class names in the
> initial <style> tag?

That - we already add the 'room-list-header' class there, so it makes sense to use the same place to add 'list-row' and 'activatable'.

Created attachment 313233 [details] [review]
ui: Highlight header on hover if sensitive

In case of account errors, room headers become clickable, but we currently
don't convey that to the user. So highlight sensitive headers on hovers
to make this more obvious.

Okay, hopefully I did not get rid of the commit, this time. :)

Attachment 313233 [details] pushed as 010fd09 - ui: Highlight header on hover if sensitive

That one's fine, thanks!

Created attachment 313235 [details] [review]
ui: Highlight room header while popover is shown

Commit 010fd09 added a hover highlight to indicate that the header
is interactive, however this does not help to visually root the
popover when shown - add a separate style when the header is toggled
for that purpose.

... so here's the :checked state in case we want this as well.

(In reply to Florian Müllner from comment #10)
> Bastian, do we want to add a style when the popover is up as well (i.e.
> :checked)?

Definitely a good idea (that would make the behavior consistent with other buttons in the UI).

Attachment 313235 [details] pushed as d6fa2eb - ui: Highlight room header while popover is shown