Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: allow table columns to specify overflow #5222

Merged
merged 2 commits into from Dec 12, 2023
Merged

fix: allow table columns to specify overflow #5222

merged 2 commits into from Dec 12, 2023

Conversation

deboer-tim
Copy link
Collaborator

@deboer-tim deboer-tim commented Dec 11, 2023

What does this PR do?

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.

Screenshot / video of UI

Self-explanatory; popup menus don't appear.

What issues does this PR fix or reference?

Fixes #5220.

How to test this PR?

Test popup menus in Images list.

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>
@deboer-tim deboer-tim requested review from benoitf and a team as code owners December 11, 2023 21:04
@deboer-tim deboer-tim requested review from dgolovin and lstocchi and removed request for a team December 11, 2023 21:04
@deboer-tim deboer-tim mentioned this pull request Dec 11, 2023
Copy link
Contributor

@lstocchi lstocchi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@jeffmaury jeffmaury left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@benoitf
Copy link
Collaborator

benoitf commented Dec 12, 2023

it is missing a test to check that overflow is added (or not)

@odockal
Copy link
Contributor

odockal commented Dec 12, 2023

Works for me locally on Linux.

Randomly picked Age column to allow overflow; test that the overflow is
hidden on every other cell but not the age column cells.

Signed-off-by: Tim deBoer <git@tdeboer.ca>
@deboer-tim
Copy link
Collaborator Author

it is missing a test to check that overflow is added (or not)

I spent a few minutes yesterday trying to think of a way to test something visually without thinking of this more obvious answer. :(. Test for overflow-hidden class added.

@benoitf benoitf merged commit 37f0572 into main Dec 12, 2023
8 checks passed
@benoitf benoitf deleted the column-overflow branch December 12, 2023 20:00
@benoitf benoitf mentioned this pull request Dec 12, 2023
5 tasks
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>
@podman-desktop-bot podman-desktop-bot added this to the 1.7.0 milestone Dec 12, 2023
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Popup menus not appearing in Images and Volumes lists
6 participants