Services filtering and other stuff #11241

marusak · 2019-02-24T16:24:04Z

This PR implements #10010 and #11237.
This PR is WIP and I am happy to improve further if design moves (especially in #11237).
I am opening as there are a lot of questions for design that rose as I was implementing this. (See screenshots at the end, there are the issues visible). Mostly asking @andreasn as he is reporter of both issues, but every single option would be great!

How should the page look like when there are no matches for current filters? (I put there just something very fast, I don't like it that the page can be blank)
Under Timers there is another button, should it be on far right or left? (Makes sense to have on left, but rather asking)
On filtering columns can resize and they often do as for example you filter our some service with very long name. Is this okay, or not? If not, what can be done?
Label Automatic Startup is rather long. On PC screen it is fine, problem is on small screens. If I put there wrapping then it wraps on PC screen (surely can be limited by screen size). Would for example Autostart be suitable?
Label All services or some state for empty search (sth. like No services...) can be confusing as there are button Targets, Sockets, Timers, Paths and you would filter them by All services. And also it is filtering autostart options.
I am confused a bit by Templates behavior (very likely I misunderstood templates) - but now they can be under all Enabled/Disabled/Static. Would it make sense to make it own "class"?

Screenshots:

TODOs:

andreasn · 2019-02-25T13:08:47Z

How should the page look like when there are no matches for current filters? (I put there just something very fast, I don't like it that the page can be blank)

Good question. I didn't consider this!
The Patternfly design guidelines on filters suggests something like this:

andreasn · 2019-02-25T13:12:23Z

4\. Label `Automatic Startup` is rather long. On PC screen it is fine, problem is on small screens. If I put there wrapping then it wraps on PC screen (surely can be limited by screen size). Would for example `Autostart` be suitable?

I went for Automatic Startup since it matches what we show on the Services details page where it forms the sentence "Automatic Startup Enabled". It also feels like more natural language to me.
I think it's OK if the header label breaks into two lines on smaller screens.

andreasn · 2019-02-25T13:15:09Z

Label All services or some state for empty search (sth. like No services...) can be confusing as there are button Targets, Sockets, Timers, Paths and you would filter them by All services. And also it is filtering autostart options.

Do you think it could work to relabel the dropdown "All Timers" etc. on the Timers page (and same for all the other pages). I realize this ends up being a bunch more work, but it would reflect the notion that the filter only applies to the current page.

andreasn · 2019-02-25T13:17:05Z

I am confused a bit by Templates behavior (very likely I misunderstood templates) - but now they can be under all Enabled/Disabled/Static. Would it make sense to make it own "class"?

I honestly don't understand how systemd templates work. Maybe @martinpitt can shed some light.

martinpitt · 2019-02-25T13:37:28Z

Yeah, maybe templates should have their own class. You can enable/disable/start/stop a template, which will create instances -- But you always have to give it an actual instantiation value, so this requires a dialog.

I. e. it makes no sense to start/stop getty@.service, as the template needs an actual terminal device. You can start/enable two instances like getty@tty1 and getty@ttyS0, and then stop/disable these.

Please see "man systemd.unit" for details about templates, but in short it's a way to build a family of similar services which differ only in a parameter (usually a device name or something such).

marusak · 2019-02-25T13:59:46Z

Thank you @andreasn for answers, I will incorporate them.
Also thank you @martinpitt for explaining templates. I understood them in the same way, still does not explain though why should getty@.service be now under enabled , httpd@.service under disabled and sshd@.service in static. Anyway, should it become 4th option in the dropdown menu? And also what should be written in column State for templates? (Template, nothing?) @andreasn

martinpitt · 2019-02-25T14:05:41Z

still does not explain though why should getty@.service be now under enabled , httpd@.service under disabled and sshd@.service in static

Right, that does make no sense. Templates by themselves are nothing by themselves, we should only show the instances. These can be enabled or static.

martinpitt · 2019-02-25T14:06:14Z

Sorry, I meant "show the instances in their respective categories". Templates themselves can/should of course still be shown, but in their separate category.

andreasn · 2019-02-25T14:09:23Z

Does it currently only filter based on Id's? If so, it's probably best to mention that in the filter inputs placeholder text to give a hint of how the search will function.
What about Filter by id rather than Type to filter...

andreasn · 2019-02-25T14:12:16Z

3\. On filtering columns can resize and they often do as for example you filter our some service with very long name. Is this okay, or not? If not, what can be done?

I played around with this now. It jumps around a little bit, but it's not too bad.
The only solution I can think of to fixing the jumping would be to give the columns a fixed width as a percentage of the page, so that the service-unit-description always gets, say 35% of the screen estate.

marusak · 2019-02-25T14:19:54Z

Sorry, I meant "show the instances in their respective categories". Templates themselves can/should of course still be shown, but in their separate category.

A lot of confusion between category, class, type... :)
Maybe would be great if @andreasn could draw design. It would make sense to have two tables in each category (Timers, Services...) one for real instances and one for templates. But that would be step back. Or make another category Templates and put it next to Paths? But Templates can be for timers or services (again two tables?). Or put it into autostart (makes less sense, but could be the least confusing?)

Does it currently only filter based on Id's?

Ids and description. But yes, design states also having there Filter by id or description... so I will go with it.

The only solution I can think of to fixing the jumping would be to give the columns a fixed width as a percentage of the page, so that the service-unit-description always gets, say 35% of the screen estate.

That would be a solution. Kinda hard to guess the widths though :) But surely I can play with it and see if there is something reasonable that can be used. (Mind that timers need custom widths as there are more columns)

andreasn · 2019-02-25T14:26:57Z

@garrett pointed out that the text for the no-results-found state was a little too complex, so changing that to No Matching Results

andreasn · 2019-02-25T16:30:42Z

On filtering columns can resize and they often do as for example you filter our some service with very long name. Is this okay, or not? If not, what can be done?

With the help of @garrett I figured out a way to make the columns not jump around so much.
Just drop this into pkg/systemd/services.css:

#services-list table {
    table-layout: fixed;
}

.service-unit-description {
    width: 40%;
}

It will of course still jump a little bit when the web browser adds and removes the scrollbar due to the page height, but that tolerable.

andreasn · 2019-02-25T16:56:46Z

I just noticed that the text is centered here instead of left aligned, but I can't quite figure out why.

andreasn · 2019-02-25T16:58:23Z

It probably makes most sense to spin off any changes to Templates into it's own issue. I don't think it's really blocking this PR.

andreasn · 2019-02-25T17:08:28Z

Ids and description. But yes, design states also having there Filter by id or description... so I will go with it.

Awesome. I just realized that the description seems case-sensitive. Is there a quick way to make it not case-sensitive? That would make searching for the description easier.

andreasn · 2019-02-25T17:16:11Z

Seems the dropdown centers the text due to the btn class in Patternfly.
It is solved elsewhere with a pull-left class on the span that surrounds the text label. Not super-elegant, and @garrett is working on a new dropdown widget in #11155 that we can use instead, where that wouldn't be needed. Do we want to block on that PR landing first?

marusak · 2019-02-25T17:43:11Z

With the help of @garrett I figured out a way to make the columns not jump around so much.

Awesome! Thanks :)

It probably makes most sense to spin off any changes to Templates into it's own issue. I don't think it's really blocking this PR.

Agree. I will open issue after this one gets merged (or at least design stabled).

Is there a quick way to make it not case-sensitive?

Sure, adding into TODO.

Not super-elegant, and @garrett is working on a new dropdown widget in #11155 that we can use instead, where that wouldn't be needed. Do we want to block on that PR landing first?

It is developed in React. Services page is not in React yet.

andreasn · 2019-02-26T09:40:17Z

It is developed in React. Services page is not in React yet.

Ah, ok. Let's use the pull-left class on the span then.

garrett · 2019-02-26T09:57:24Z

It will of course still jump a little bit when the web browser adds and removes the scrollbar due to the page height, but that tolerable.

It's possible to make the page not jump in that case, with overflow-y: scroll on the element that will scroll. (It's often, but not always, body.)

You could, for example, set overflow-y: scroll; on body when you're going to filter and remove it when you're not filtering. Or we could always set that attribute for the page, as it's usually a long table-based list of items.

garrett · 2019-02-26T09:58:14Z

Ah, ok. Let's use the pull-left class on the span then.

It would most likely be better to use display: flex (or display: grid, depending). I can help with this, if you'd like. 😉

BTW: The new dropdown widget is a drop-in replacement for React components, but copy/pasting the HTML (when it is done and merged) should be possible, as the CSS will be in the global CSS used for Cockpit.

garrett

Issues I've found in this PR

There's an empty <div> around "create-timer". (See inline comment.)
Hitting escape doesn't clear the filter box.
There's no label for the filter with a for="Element_ID_goes_HERE" or aria-labelledby (as a fallback for elements without a label but with text somewhere) — or, in this case, as there's no element visibly saying "Filter", an aria-label.
Filter isn't focused by default (this is debatable, but probably would be nice feature-wise — there's a possible accessibility issue by skipping the toggle buttons on the left, however).

This last one is debatable. I'm fine if we talk about this and if it comes later. Or we could discuss it and it would be implement The other items on the list should be fixed, however.

As noted above, the dropdown has some issues, but we're hoping to solve those elsewhere (and backport it to the non-React parts of Cockpit, like here).

Suitable for another PR

Filter bar behaves oddly in small viewports. As the list is also problematic in small viewports, and this is an existing problem (although now excerbated with the filtering), I'm happy with it as another PR.

garrett · 2019-02-26T13:46:21Z

pkg/systemd/services.html

@@ -56,13 +59,27 @@
        <button class="btn btn-default" role="tab" data-pattern="\.timer$" translatable="yes">Timers</button>
        <button class="btn btn-default" role="tab" data-pattern="\.path$" translatable="yes">Paths</button>
      </div>
-      <button id="create-timer" data-toggle="#timer-dialog" data-target="#timer-dialog" class="btn btn-primary pull-right" translate>Create Timer</button>
+      <div>


There's an empty <div> here. Usually this comes up with React, where <React.Fragment> should be used. I'm not sure why there's an empty div here though. If you need to group elements in a wrapper, try to use a semantic element (when it makes sense) and please at least give it a class and/or ID regardless to describe why this is here. (Something like "services-filter" might work.)

andreasn · 2019-02-26T14:48:04Z

As noted above, the dropdown has some issues, but we're hoping to solve those elsewhere (and backport it to the non-React parts of Cockpit, like here).

What do you think is the timeline for the non-react-dropdown fixes? Is it blocking this PR on fixing that first, or should we do the pull-left hack?

garrett · 2019-02-26T15:09:45Z

What do you think is the timeline for the non-react-dropdown fixes?

I need to include the CSS.
- To do so, I need to include an SVG somehow. @larskarlitski and I are looking into this. @martinpitt might need to look into it too when he's back.
Then it needs to be tweaked from the CSS side of things and tested everywhere.

Is it blocking this PR on fixing that first, or should we do the pull-left hack?

No, it's not blocking on the dropdown.

We should all avoid pull-left and using floats for layout (especially floats that change display order from visible content order). It's just bad code and a bad concept.

I don't think it has anything to do with the dropdown though? There are other ways to solve it?

garrett · 2019-02-26T15:24:44Z

Oh, I looked at it, and pull-left is fine in this usage, as:

it doesn't change order of elements
it is already contained (so it wouldn't escape the parent element)

Go for it. 👍

marusak · 2019-02-26T16:36:42Z

Thank you guys for all the comments and ideas!!
I went through all of them and fixed them. I pushed new version. I will comment on few things, what I do not comment on is done.

There's an empty <div> here. Usually this comes up with React, where <React.Fragment> should be used. I'm not sure why there's an empty div here though

It is to make it work with flex. I want some controls to be on left and some on right. So I split it into two divs, put there justify-content: space-between. I guess it can be done better. Anyway put there class to make it at least a bit more understandable.

Hitting escape doesn't clear the filter box.

I either don't understand what you are trying to do, or cannot reproduce. I can write text into filter box and then delete it just fine with escape - it is standard input with no magic at all.

Filter isn't focused by default (this is debatable, but probably would be nice feature-wise — there's a possible accessibility issue by skipping the toggle buttons on the left, however).

I don't have any strong options about this. Kinda it feels strange to focus search field (especially when there are other selectors left of them). But I am fine with implementing it if I am told so.

The new dropdown widget is a drop-in replacement for React components, but copy/pasting the HTML (when it is done and merged) should be possible, as the CSS will be in the global CSS used for Cockpit.

Sounds well, but I would not do it in this PR. It should be separate PR that will do it on all non-react place of Cockpit.

Anyway, anything discussed here should be fixed. I also went through and used listing instead of what we currently had. It works nicely with small screens (current the table breaks very weirdly on small screens) buuut there are two isses - one is in design and now sure if can be fixed (possible reason why this wont work and other solution needs to be found) other is just styling.

First issue - between two last columns there is that big gap - it is because second to last column floats left and last to right. Is this a problem? What style can be used to improve this?

Second issue - On smaller screens columns can break layout - notice how last items second column jumps to left.

marusak · 2019-02-26T16:51:30Z

A bit of thinking about the second issue - the breaking layout. Possibly grid, where each column would be some percentage could work. We would need a few classes as different tables may have different number of columns, but surely could be done. Ideas?

garrett · 2019-02-26T17:30:34Z

I either don't understand what you are trying to do, or cannot reproduce. I can write text into filter box and then delete it just fine with escape - it is standard input with no magic at all.

I simply typed into the box and hit the ESC key. Nothing happened. It should clear the entry (but keep focus).

The layout fixing should be in another PR, hence me opening up #11258 earlier today. It's too complex to block this PR, and it is an existing problem. It's probably tricky with lots of HTML/CSS changes too. (The HTML changes would most likely be attribute related, not massive restructuring.)

More details about making accessible tables are in #11260. But I guess that's more for react tables and we'd handle services similarly, but sadly as a one-off, as it isn't React based.

martinpitt

Just one bug away from landing, from my POV. The page works really well now, great work!

martinpitt · 2019-02-28T17:10:50Z

pkg/systemd/services.html

+  <script id="service-empty-tmpl" type="x-template/mustache">
+    <div id="empty-search" class="blank-slate-pf blank-screen">
+      <h1 translate>
+        No Matching Results


The whitiespacing doesn't work, it gets copied literally to cockpit.pot:

msgid " No Matching Results "

Please write it as <h1>Bla</h1>.

Ach, did not notice that. Thank you for pointing this out. Repushed

martinpitt · 2019-02-28T17:17:29Z

pkg/systemd/services.html

@@ -62,6 +62,19 @@
      <div class="filter-group">
        <button id="create-timer" data-toggle="#timer-dialog" data-target="#timer-dialog" class="btn btn-primary" translate>Create Timer</button>
        <input type="search" id="services-text-filter" class="form-control" aria-label="Filter" placeholder="Filter by Id or description..." translate="placeholder"/>
+        <div class="dropdown" id="services-dropdown">


FTR, in the spirit of #11155 this should become an actual <select> soon. Right now the selector isn't keyboard navigatable at all. Fine for a follow-up of course.

Fixes: cockpit-project#10010

Use one table and autostart option move to the table. Design: cockpit-project#11237 Fixes: cockpit-project#11237

Fixes: cockpit-project#11237

This prevents small jumps while filtering as on few results scroll bar is removed and content resized.

Keeping table for better accessibility. Follows design from cockpit-project#11237

Services are ordered by ids and if the this column is second users are not able to identify how the list is ordered. Design: cockpit-project#11237

Fixes cockpit-project#698 Closes cockpit-project#11241

martinpitt

Thanks for the tabindex change, that should help!

garrett · 2019-03-04T09:21:42Z

pkg/systemd/services.html

+        <li class="active" aria-current=true tabindex="0" data-pattern="\.service$"><a translatable="yes">System Services</a></li>
+        <li tabindex="0" data-pattern="\.socket$"><a translatable="yes">Sockets</a></li>
+        <li tabindex="0" data-pattern="\.timer$"><a translatable="yes">Timers</a><li>
+        <li tabindex="0" data-pattern="\.path$"><a translatable="yes">Paths</a><li>


Ah, cool to see tabindex as the tabs need to be selected — but why is it in <li> instead of <a>? (Better yet would be to add href to the <a>, instead of using a tabindex, and use a URL fragment* to remember the state in the URL.)

* URL fragment (#this-thing-here), not to be confused with a Document Fragment, React.Fragment, CSS Fragmentation, or anything else called "fragment" in webdev. 😉

garrett requested changes Feb 26, 2019

View reviewed changes

garrett mentioned this pull request Feb 26, 2019

Services: Improve mobile layout for filter bar and list #11258

Closed

3 tasks

marusak force-pushed the services_filters branch from 93871b8 to ec874d4 Compare February 26, 2019 16:20

marusak force-pushed the services_filters branch from 278c417 to d155407 Compare February 28, 2019 11:25

marusak mentioned this pull request Feb 28, 2019

Enhance showing units' templates #11269

Closed

marusak force-pushed the services_filters branch from d155407 to e333e83 Compare February 28, 2019 11:55

marusak force-pushed the services_filters branch from e333e83 to 4da0f11 Compare February 28, 2019 12:54

marusak force-pushed the services_filters branch from 4da0f11 to bd37b3d Compare February 28, 2019 13:25

martinpitt requested changes Feb 28, 2019

View reviewed changes

marusak and others added 12 commits March 1, 2019 06:54

services: Enable filtering content of services

a3fbd68

Fixes: cockpit-project#10010

services: Do not split into 3 tables, but use one

7810c26

Use one table and autostart option move to the table. Design: cockpit-project#11237 Fixes: cockpit-project#11237

services: Enable filtering by autostart

bdbec7b

Fixes: cockpit-project#11237

services: Always show scroll bar

8419b27

This prevents small jumps while filtering as on few results scroll bar is removed and content resized.

services: Style table to look like listing

8bde893

Keeping table for better accessibility. Follows design from cockpit-project#11237

services: Put Id as first column

b2d793d

Services are ordered by ids and if the this column is second users are not able to identify how the list is ordered. Design: cockpit-project#11237

services: Improve service list accessibility

3a4eabd

systemd: Use sticky for the content-header-extra widget

1930ee5

services: Force-wrap long service strings

b06c558

service: Add classes to rows

3b24103

service: Align colors to PatternFly (and AA contrast for headers)

29b40e6

services: Use tabs instead button groups

f77d03f

Fixes cockpit-project#698 Closes cockpit-project#11241

marusak force-pushed the services_filters branch from bd37b3d to f77d03f Compare March 1, 2019 05:55

martinpitt approved these changes Mar 1, 2019

View reviewed changes

martinpitt merged commit 1035cbe into cockpit-project:master Mar 1, 2019

marusak mentioned this pull request Mar 1, 2019

[no-test] Services - bunch or random ideas #11273

Closed

garrett reviewed Mar 4, 2019

View reviewed changes

This was referenced Mar 4, 2019

[no-test] Accessible navtabs in services #11295

Closed

services: Change filter dropdown to a native select #11334

Merged

Services filtering and other stuff #11241

Services filtering and other stuff #11241

Conversation

marusak commented Feb 24, 2019 • edited Loading

andreasn commented Feb 25, 2019

andreasn commented Feb 25, 2019

andreasn commented Feb 25, 2019

andreasn commented Feb 25, 2019

martinpitt commented Feb 25, 2019

marusak commented Feb 25, 2019

martinpitt commented Feb 25, 2019

martinpitt commented Feb 25, 2019

andreasn commented Feb 25, 2019

andreasn commented Feb 25, 2019

marusak commented Feb 25, 2019 • edited Loading

andreasn commented Feb 25, 2019

andreasn commented Feb 25, 2019

andreasn commented Feb 25, 2019

andreasn commented Feb 25, 2019

andreasn commented Feb 25, 2019

andreasn commented Feb 25, 2019

marusak commented Feb 25, 2019 • edited Loading

andreasn commented Feb 26, 2019

garrett commented Feb 26, 2019

garrett commented Feb 26, 2019 • edited Loading

garrett left a comment

Choose a reason for hiding this comment

Issues I've found in this PR

Suitable for another PR

garrett Feb 26, 2019

Choose a reason for hiding this comment

andreasn commented Feb 26, 2019

garrett commented Feb 26, 2019

garrett commented Feb 26, 2019

marusak commented Feb 26, 2019

marusak commented Feb 26, 2019

garrett commented Feb 26, 2019

martinpitt left a comment

Choose a reason for hiding this comment

martinpitt Feb 28, 2019

Choose a reason for hiding this comment

marusak Mar 1, 2019

Choose a reason for hiding this comment

martinpitt Feb 28, 2019

Choose a reason for hiding this comment

martinpitt left a comment

Choose a reason for hiding this comment

garrett Mar 4, 2019

Choose a reason for hiding this comment

marusak commented Feb 24, 2019 •

edited

Loading

marusak commented Feb 25, 2019 •

edited

Loading

marusak commented Feb 25, 2019 •

edited

Loading

garrett commented Feb 26, 2019 •

edited

Loading