Volume table component #4841
Merged
Volume table component #4841
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
deboer-tim
requested review from
jeffmaury and
feloy
and removed request for
a team
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>
deboer-tim
force-pushed
the
volume-table
branch
from
35e7b6f
to
2261150
Compare
|
Rebased on main. |
|
The order seems reverted, at least on Name and Age columns. When a
|
|
With content larger than the cell, the content is overlapping through the other columns.
|
Set a max width so that cells don't overlap and the overflow-hidden can work correctly. Signed-off-by: Tim deBoer <git@tdeboer.ca>
|
I've set a max width so that table cells do not overlap anymore; they will be cropped. Now that we're not based on html table I think there is a lot more that we could do here in the future (e.g. correctly use '...' in the cell, or setting minimum widths), but this initial change fixes the ugliness. The sorting direction felt natural to me at the time, but you are correct. However, it is mostly independent to this PR, the image PR, and the PR for sort indicators, and I'm starting to have trouble with all of them touching the same file. :) Are you ok splitting it off into a separate issue and fixing it 'later'? I've opened #4881 to track this, and would be able to PR quickly as soon as one or two of these others get merged. |
|
copy/pasta from dev chat, re: the arrows and ordering: ooh this is a perennial problem 🙂 does the arrow (or any other icon that also can be read as status) mark the action to be taken by clicking on it, or the current state of things. I would argue in this case, the up arrow marks the action to be taken, and you can make that clear by keeping the downward facing error but greying it out (it's disabled since you can't sort the other way as its already sorted that way) |
|
I’ve tried to cover improved sorting for the component in #4881 so that this can focus just on moving to the component for the Volume page. |
deboer-tim
added a commit
that referenced
this pull request
Nov 22, 2023
These changes are already proposed in PRs #4844 and #4841 respectively, but since those PRs are taking a while in review and these changes are required for other PRs I'm breaking them out here: - Add missing aria roles to the first two cells in the column. (Not only is this correct, but lack of them breaks e2e tests). - Add max-w-full to each cell, so that the overflow-hidden applies correctly and cells that are too narrow are cropped instead of overlapping. Signed-off-by: Tim deBoer <git@tdeboer.ca>
feloy
reviewed
Nov 22, 2023
deboer-tim
added a commit
that referenced
this pull request
Nov 23, 2023
These changes are already proposed in PRs #4844 and #4841 respectively, but since those PRs are taking a while in review and these changes are required for other PRs I'm breaking them out here: - Add missing aria roles to the first two cells in the column. (Not only is this correct, but lack of them breaks e2e tests). - Add max-w-full to each cell, so that the overflow-hidden applies correctly and cells that are too narrow are cropped instead of overlapping. Signed-off-by: Tim deBoer <git@tdeboer.ca>
deboer-tim
added a commit
that referenced
this pull request
Nov 23, 2023
Now that both #4841 (Volume table component) and #4860 (default sort column) have merged, we can follow up and mark the default sort column for volumes: name. This change has no effect other than setting the sort indicator for the Volumes column by default. Signed-off-by: Tim deBoer <git@tdeboer.ca>
deboer-tim
added a commit
that referenced
this pull request
Nov 24, 2023
Now that both #4841 (Volume table component) and #4860 (default sort column) have merged, we can follow up and mark the default sort column for volumes: name. This change has no effect other than setting the sort indicator for the Volumes column by default. Signed-off-by: Tim deBoer <git@tdeboer.ca>
deboer-tim
added a commit
that referenced
this pull request
Dec 11, 2023
With the previous table (<1.6), column widths were variable and this caused problems: - sometimes a long image name would cause the tables to require horizontal scrolling - sometimes a change in one column would cause other columns to shift E.g. starting a container can cause the start/stop button to jump right, and stopping the container the button jumps left, so you might click the wrong button (e.g. delete). With the new grid-based table component, column widths are fixed so content doesn't affect table width. This solves the previous issues, but it means that simple columns with long content are more likely to have overflow. This was 'fixed' in #4841 by setting a maximum column width and overflow-hidden to limit rendering to within the cell, but now things like popup menus can't go outside of the cells. I've struggled deciding what the correct default behaviour should be, but I still think #4841 is correct and means the average renderer can stay simple. This change adds a new 'overflow' property to allow renderers that are more interesting to control their own behaviour and render 'outside' of their cell. Property added to both image and volume action columns. Fixes #5220. Signed-off-by: Tim deBoer <git@tdeboer.ca>
benoitf
pushed a commit
that referenced
this pull request
Dec 12, 2023
* fix: allow table columns to specify overflow With the previous table (<1.6), column widths were variable and this caused problems: - sometimes a long image name would cause the tables to require horizontal scrolling - sometimes a change in one column would cause other columns to shift E.g. starting a container can cause the start/stop button to jump right, and stopping the container the button jumps left, so you might click the wrong button (e.g. delete). With the new grid-based table component, column widths are fixed so content doesn't affect table width. This solves the previous issues, but it means that simple columns with long content are more likely to have overflow. This was 'fixed' in #4841 by setting a maximum column width and overflow-hidden to limit rendering to within the cell, but now things like popup menus can't go outside of the cells. I've struggled deciding what the correct default behaviour should be, but I still think #4841 is correct and means the average renderer can stay simple. This change adds a new 'overflow' property to allow renderers that are more interesting to control their own behaviour and render 'outside' of their cell. Property added to both image and volume action columns. Fixes #5220. Signed-off-by: Tim deBoer <git@tdeboer.ca>
benoitf
pushed a commit
to benoitf/desktop
that referenced
this pull request
Dec 12, 2023
* fix: allow table columns to specify overflow With the previous table (<1.6), column widths were variable and this caused problems: - sometimes a long image name would cause the tables to require horizontal scrolling - sometimes a change in one column would cause other columns to shift E.g. starting a container can cause the start/stop button to jump right, and stopping the container the button jumps left, so you might click the wrong button (e.g. delete). With the new grid-based table component, column widths are fixed so content doesn't affect table width. This solves the previous issues, but it means that simple columns with long content are more likely to have overflow. This was 'fixed' in containers#4841 by setting a maximum column width and overflow-hidden to limit rendering to within the cell, but now things like popup menus can't go outside of the cells. I've struggled deciding what the correct default behaviour should be, but I still think containers#4841 is correct and means the average renderer can stay simple. This change adds a new 'overflow' property to allow renderers that are more interesting to control their own behaviour and render 'outside' of their cell. Property added to both image and volume action columns. Fixes containers#5220. Signed-off-by: Tim deBoer <git@tdeboer.ca>
benoitf
pushed a commit
that referenced
this pull request
Dec 12, 2023
* fix: allow table columns to specify overflow With the previous table (<1.6), column widths were variable and this caused problems: - sometimes a long image name would cause the tables to require horizontal scrolling - sometimes a change in one column would cause other columns to shift E.g. starting a container can cause the start/stop button to jump right, and stopping the container the button jumps left, so you might click the wrong button (e.g. delete). With the new grid-based table component, column widths are fixed so content doesn't affect table width. This solves the previous issues, but it means that simple columns with long content are more likely to have overflow. This was 'fixed' in #4841 by setting a maximum column width and overflow-hidden to limit rendering to within the cell, but now things like popup menus can't go outside of the cells. I've struggled deciding what the correct default behaviour should be, but I still think #4841 is correct and means the average renderer can stay simple. This change adds a new 'overflow' property to allow renderers that are more interesting to control their own behaviour and render 'outside' of their cell. Property added to both image and volume action columns. Fixes #5220. Signed-off-by: Tim deBoer <git@tdeboer.ca>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this PR do?
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.
Screenshot/screencast of this PR
Before:
After:
What issues does this PR fix or reference?
Fixes #4839.
How to test this PR?
Use the Volumes page as normal. Try sorting by each column.