Useful listing table #481

jelly · 2024-05-29T15:51:37Z

Make selectable visually work .selected-class-thing + td { background: var(--pf-v5--theBackGroundColor); }
Stop using a Card in the first column, <Icon isInline><p>File or dir</p> instead
Make the contextmenu and selecting event handlers work
Make everything sortable
Stretch goal, combine both views into one as per https://garrett.github.io/cockpit-mockweb/navigator/grid#

Re-useable table approach issues

Refactor / clean up render function
Size header is not allowed with content
Directory is empty twice
Too much spacing when there three items
wrong icon for folders
Icon view not really centered
Icon not centered / wrongly aligned
General text/font

Follow up

More useful dates? "Yesterday at 12:00" instead?
Update & Merge the PF update PR as we no longer use a which blocked us from updating
Scrollbar position wrong
Table should take the full height when available.

.pf-v5-c-sidebar__main, .pf-v5-c-sidebar__content, .pf-v5-c-card { height: 100%; }

src/files-card-body.jsx

jelly · 2024-05-30T07:52:55Z

Dropping the Card and just using a label gives us a smaller row size.

jelly · 2024-05-30T10:11:41Z

@garrett I have come quite far porting your CSS to the file viewer one big interesting detail is that our tables are a bit weird:

<tbody>
<tr>
</tr>
</tbody>
<tbody>
...

This is likely due to the ListingTable also supporting expendable content.

Adjusted our CSS, but layout is still a small problem

This means we have two options:

Adjust our CSS and make it work
Use the normal PF table elements instead of a ListingTable, this mean that we lack some ct-table stuff.

src/files-card-body.jsx

allisonkarlitskaya

Some assorted drive-bys:

allisonkarlitskaya · 2024-06-04T08:10:50Z

src/files-card-body.jsx

@@ -73,6 +74,14 @@ const compare = (sortBy) => {
                ? -1
                : 1)
            : compareFileType(a, b);
+    case "largest_size":
+        return (a, b) => (a.size > b.size
+            ? -1


I think you just want (a, b) => b.size - a.size for this. The sort function here doesn't properly handle the "two items equal" case, and it's more complicated than it needs to be, anyway.

Also: this appears not to respect "directories" first...

It does not, and in my opinion why would directories need to be first if I am sorting on size. We have no file count information or directory size information so I don't see how sorting on file size is useful in that regard by listing directories first. (@garrett )

Not going to block on this but it doesn't feel like it makes sense to me.

If you mean it doesn't handle two items equal and then sort them alphabetically? Then yes that is true :)

src/files-card-body.jsx

src/files-card-body.scss

src/files-card-body.jsx

allisonkarlitskaya

This looks great!

Some minor comments below, but feel free to handle them as follow-ups...

src/files-card-body.jsx

allisonkarlitskaya · 2024-06-04T14:27:38Z

src/files-card-body.jsx

+};
+
+// Memoize the Item component as rendering thousands of them on each render of parent component is costly.
+const Row = React.memo(function Item({ file, isSelected }) {


For the record, I don't think the memoize currently protects us from changes to unrelated files since we re-create the list on every single fsinfo event, but I'm happy to see what we are theoretically memoized, because it means that we can fix that bug and benefit from it...

It does, I tested this on /bin, the event listener took ~ 200-300ms and with memozing its a lot faster.

React can be a bit confusing :)

So re-render => props/state changes and the render function is called again forcing all children to "re-render". By memozing React does not re-run the render function as the props did not change.

Soooo you are right if fsinfo updates and generates a new Object because like setstate react compares objects on identity. So we could fix this by passing:

name

mtime

size

type

Another issue might be that we memoize the sort order of the array. But then again changing our state to an Object might help with that :)

allisonkarlitskaya · 2024-06-04T14:30:45Z

src/files-card-body.scss

+                display: none;
+            }
+
+            .item-size,


allisonkarlitskaya · 2024-06-04T14:31:07Z

src/files-card-body.scss

+            }
+
+            .item-size,
+            .item-date {


I think the inclusion of .item-date here is probably accidental since you display: none it, just below?

Howso, we want the font-size and color to be the same?

But we don't show the date...

Our card listingtable could not easily support showing columns for size, modified and name. But instead we can use a Table with CSS to accommodate both views. The added benefits of dropping the card, showing the icons via CSS outweigh using a Card or a per view specific component. The ListingTable is not used as it has a <tbody> for every row which is simply unneeded and we already require our own CSS customization for styling the Table to support both views. Co-Authored-By: Garrett LeSage <[email protected]>

cockpituous · 2024-06-04T15:15:22Z

src/files-card-body.jsx

+        onSort: (_event, index, direction) => {
+            setSortBy(filterColumns[index][direction].itemId);


These 2 added lines are not executed by any test.

allisonkarlitskaya

As a wise man once said...

allisonkarlitskaya · 2024-06-04T15:16:04Z

yolo.

jelly added the no-test label May 29, 2024

github-advanced-security bot found potential problems May 29, 2024

View reviewed changes

src/files-card-body.jsx Fixed Show fixed Hide fixed

jelly mentioned this pull request May 31, 2024

Show icons for various different file types #486

Merged

jelly force-pushed the real-table-view branch from a80d150 to 8112bab Compare June 3, 2024 09:16

github-advanced-security bot found potential problems Jun 3, 2024

View reviewed changes

src/files-card-body.jsx Fixed Show fixed Hide fixed

src/files-card-body.jsx Fixed Show fixed Hide fixed

src/files-card-body.jsx Fixed Show fixed Hide fixed

src/files-card-body.jsx Fixed Show fixed Hide fixed

github-advanced-security bot found potential problems Jun 3, 2024

View reviewed changes

src/files-card-body.jsx Fixed Show fixed Hide fixed

allisonkarlitskaya reviewed Jun 4, 2024

View reviewed changes

jelly removed the no-test label Jun 4, 2024

jelly force-pushed the real-table-view branch from cf98789 to 0bad527 Compare June 4, 2024 13:50

jelly marked this pull request as ready for review June 4, 2024 14:30

allisonkarlitskaya previously approved these changes Jun 4, 2024

View reviewed changes

jelly dismissed allisonkarlitskaya’s stale review via 9967c03 June 4, 2024 14:56

jelly force-pushed the real-table-view branch from 0bad527 to 9967c03 Compare June 4, 2024 14:56

jelly force-pushed the real-table-view branch from 9967c03 to 4d6df99 Compare June 4, 2024 15:07

jelly requested a review from allisonkarlitskaya June 4, 2024 15:07

cockpituous reviewed Jun 4, 2024

View reviewed changes

allisonkarlitskaya approved these changes Jun 4, 2024

View reviewed changes

allisonkarlitskaya merged commit 15ee985 into cockpit-project:main Jun 4, 2024
8 of 12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Useful listing table #481

Useful listing table #481

jelly commented May 29, 2024 •

edited

Loading

jelly commented May 30, 2024 •

edited

Loading

jelly commented May 30, 2024 •

edited

Loading

allisonkarlitskaya left a comment

allisonkarlitskaya Jun 4, 2024

allisonkarlitskaya Jun 4, 2024

jelly Jun 4, 2024

jelly Jun 4, 2024 •

edited

Loading

jelly Jun 4, 2024

allisonkarlitskaya left a comment

allisonkarlitskaya Jun 4, 2024

jelly Jun 4, 2024

jelly Jun 4, 2024

jelly Jun 5, 2024

allisonkarlitskaya Jun 4, 2024

allisonkarlitskaya Jun 4, 2024

jelly Jun 4, 2024

allisonkarlitskaya Jun 4, 2024

cockpituous Jun 4, 2024

allisonkarlitskaya left a comment

allisonkarlitskaya commented Jun 4, 2024

		onSort: (_event, index, direction) => {
		setSortBy(filterColumns[index][direction].itemId);

Useful listing table #481

Useful listing table #481

Conversation

jelly commented May 29, 2024 • edited Loading

Re-useable table approach issues

Follow up

jelly commented May 30, 2024 • edited Loading

jelly commented May 30, 2024 • edited Loading

allisonkarlitskaya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jelly Jun 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allisonkarlitskaya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allisonkarlitskaya left a comment

Choose a reason for hiding this comment

allisonkarlitskaya commented Jun 4, 2024

jelly commented May 29, 2024 •

edited

Loading

jelly commented May 30, 2024 •

edited

Loading

jelly commented May 30, 2024 •

edited

Loading

jelly Jun 4, 2024 •

edited

Loading