feat: table component #4545
feat: table component #4545
Conversation
deboer-tim
force-pushed
the
table-comp
branch
from
da30e4c
to
028d781
Compare
|
Updated commit to make columns named instead of numbered, and a little cleanup and renaming. Still not entirely sure how the column/row styling should work in the Column interface, still iterating there. Added sorting just to prove it was possible. There is no sort indicator on the column header yet and a few columns should sort by value instead of alphabetically, but it works. |
deboer-tim
force-pushed
the
table-comp
branch
from
028d781
to
7dade74
Compare
|
hi, from what I've seen, and I think @cdrage did some investigations long time ago, we should use grid system and not table |
|
Pinged Charlie, he's checked all his old branches and forks and can't find any of the code from that investigation. I'm somewhat hesitant to switch to grid here since that will hold this up and do 'multiple things' in one PR... but adopting this on a page is also the ideal time to switch, so I'll give it a try. |
deboer-tim
force-pushed
the
table-comp
branch
from
7dade74
to
bfda795
Compare
|
Updated description, added video, and force-pushed the latest rebased on main. A lot has changed since the last update and would appreciate feedback so that I can address issues before this moves out of draft state. |
deboer-tim
force-pushed
the
table-comp
branch
from
bfda795
to
e3a635c
Compare
There was a problem hiding this comment.
Amazing! I'm able to actually organize it by age / alphabetical whenever I want. so big 👍
With regards to table helper, I'm assuming we have to make a new helper for each list scenario for Pods / Containers / etc?
I'm good with the concept of using TableHelper to make it generic and then "extending" it for different use cases. As honestly it seems like the best way to make it as "generic" as possible considering how difficult it is with other tables / etc.
I'm assuming this will scale well with the other lists we are implementing too
|
Added new commit: Lots of changes based on review comments:
Overall I like these changes; simpler implementation and easier/safer to use for consumers. I am still having trouble with the grid layout misbehaving though. |
deboer-tim
force-pushed
the
table-comp
branch
from
eef6cd2
to
22f4731
Compare
deboer-tim
force-pushed
the
table-comp
branch
from
0e604c3
to
7808df3
Compare
This is a clean branch of the table component after previous review comments in PR #4545. Each page passes an array of objects and Columns to the Table component, which is responsible for creating the basic layout and calling back to the Column renderer to render each cell in the table. The Table uses grid layout and provides enough abstraction that we can implement additional features later. (e.g. optional columns, column reordering, or resizing. grouping. etc) The Table provides sorting & sort indicators, and tests are included. Support for 'object containers/grouping' in order to support Container list will be done separately since this is already a big PR. Fixes #4365. Signed-off-by: Tim deBoer <git@tdeboer.ca>
deboer-tim
force-pushed
the
table-comp
branch
from
20b7e8c
to
15dbe75
Compare
deboer-tim
requested review from
dgolovin and
lstocchi
and removed request for
a team
|
Clean branch (without Images/Volumes) pushed, PR comment updated, and draft status removed. Ready for final review. |
Switches the Volume page to the new table component. Columns are extracted as expected, and I've pulled out the engine into an Environment column to match the Pods page. I made all the columns sortable. The raw size had to be made visible from VolumeInfoUI because the regular column is humanized and not sortable. Status and size are 'reverse sorted' because you typically want to see used volumes and bigger things first. Based on the Table PR #4545. Ignore everything in /table/* and once that has merged I'll rebase. Signed-off-by: Tim deBoer <git@tdeboer.ca>
Switches the Images page to the new table component. Columns are extracted as expected, and I've pulled out the engine into an Environment column to match the Pods page. I made all the columns sortable. The raw size had to be made visible from ImageInfoUI because the regular column is humanized and not sortable. Status and size are 'reverse sorted' because you typically want to see used images and bigger things first. Based on the Table PR #4545. Ignore everything in /table/* and once that has merged I'll rebase. Signed-off-by: Tim deBoer <git@tdeboer.ca>
| const toggleData = data; | ||
| toggleData.filter(object => row.selectable?.(object)).forEach(object => (object.selected = checked)); | ||
| data = toggleData; |
There was a problem hiding this comment.
What is the purpose of using the toggleData temp variable? As toggleData is a reference to data, it does not seem necessary to me
There was a problem hiding this comment.
You can see this was actually unchanged from before. Svelte only detects changed objects, not internal state, so sometimes you have to explicitly assign something to see the change visually. I did some testing and this doesn't seem to be required anymore, so I removed it.
The same thing existed further down on line 64 with sorting. This one is required, but I can see the same question arising so I changed it to an explicit 'data = data.sort()', and added the required no-mutation comment. I don't think this is any better code-wise, but at least it's obvious it was intentional.
There was a problem hiding this comment.
Now that you tell me the reason, I remember I effectively had to write some data = [...data] in some other PR, to make the refresh happen. Not sure what is the recommended way to handle this
There was a problem hiding this comment.
In this article, it is explained that data = data is enough to trigger the Svelte changes.
https://javascript.plainenglish.io/update-arrays-and-objects-in-svelte-40c6a84f8a28
There was a problem hiding this comment.
I've seen it elsewhere too so it seem to be a common way of handling it. For now though sort is the only place that needs it and eslint comment make it clear the code is intentional.
Changes: - Moved comparator into ColumnInformation to make things more functional. - Moved Row selection and disabledText into RowInformation object; now everything matches as well. - Last item required me to put in a few more undefined checks and I questioned why we'd have a checkbox column if items aren't selectable, so I removed it entirely in that case (i.e. if there is no selectable then the checkbox column is removed entirely). - Removed unnecessary assignment to toggleData and replaced sort with reassignment (and es-lint mutation override). - Added missing type in TestTable. Signed-off-by: Tim deBoer <git@tdeboer.ca>
| // number of selected items in the list | ||
| export let selectedItemsNumber: number = 0; | ||
| $: selectedItemsNumber = row.info.selectable | ||
| ? data.filter(object => row.info.selectable?.(object)).filter(object => object.selected).length |
There was a problem hiding this comment.
any reason why you don't merge the two filters? data.filter(object => row.info.selectable?.(object) && object.selected)
There was a problem hiding this comment.
None at all, I just copied from ImagesList and VolumesList and didn't think to fix. :) I'll do it in follow-up PR.
This is a clean branch of the table component after previous review comments in PR #4545. Each page passes an array of objects and Columns to the Table component, which is responsible for creating the basic layout and calling back to the Column renderer to render each cell in the table. The Table uses grid layout and provides enough abstraction that we can implement additional features later. (e.g. optional columns, column reordering, or resizing. grouping. etc) The Table provides sorting & sort indicators, and tests are included. Support for 'object containers/grouping' in order to support Container list will be done separately since this is already a big PR. Fixes #4365. Signed-off-by: Tim deBoer <git@tdeboer.ca>
deboer-tim
dismissed
cdrage’s stale review
Changes were addressed, two other approvals, and Charlie on vacation.
This is a clean branch of the table component after previous review comments in PR #4545. Each page passes an array of objects and Columns to the Table component, which is responsible for creating the basic layout and calling back to the Column renderer to render each cell in the table. The Table uses grid layout and provides enough abstraction that we can implement additional features later. (e.g. optional columns, column reordering, or resizing. grouping. etc) The Table provides sorting & sort indicators, and tests are included. Support for 'object containers/grouping' in order to support Container list will be done separately since this is already a big PR. Fixes #4365. Signed-off-by: Tim deBoer <git@tdeboer.ca>
Switches the Volume page to the new table component. Columns are extracted as expected, and I've pulled out the engine into an Environment column to match the Pods page. I made all the columns sortable. The raw size had to be made visible from VolumeInfoUI because the regular column is humanized and not sortable. Status and size are 'reverse sorted' because you typically want to see used volumes and bigger things first. Based on the Table PR #4545. Ignore everything in /table/* and once that has merged I'll rebase. Signed-off-by: Tim deBoer <git@tdeboer.ca>
Switches the Volume page to the new table component. Columns are extracted as expected, and I've pulled out the engine into an Environment column to match the Pods page. I made all the columns sortable. The raw size had to be made visible from VolumeInfoUI because the regular column is humanized and not sortable. Status and size are 'reverse sorted' because you typically want to see used volumes and bigger things first. Based on the Table PR #4545. Ignore everything in /table/* and once that has merged I'll rebase. Signed-off-by: Tim deBoer <git@tdeboer.ca>
What does this PR do?
This is a clean, rebased branch of the table component after previous review comments in this PR.
We have impending technical debt where our expanding number of table components (images, volumes, pods, containers, soon adding Kubernetes objects) will cross with our future design ideas (tables should be sortable, columns configurable, environments groupable, etc). We can't copy/paste these features into every page, and deal with differences between them.
Each page passes an array of objects and Columns to the Table component, which is responsible for creating the basic layout and calling back to the Column renderer to render each cell in the table. The Table uses grid layout and provides enough abstraction that we can implement additional features later. (e.g. optional columns, column reordering, or resizing. grouping. etc)
The Table provides sorting & sort indicators, and tests are included. Support for 'object containers/grouping' in order to support Container list will be done separately since this is already a big PR.
Screenshot/screencast of this PR
Before:
After:
What issues does this PR fix or reference?
Fixes #4365.
How to test this PR?
PR tests; separate PRs for Images and Volumes coming shortly.