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

Issue 5311 - Add/remove columns in tickets table #5402

Open
wants to merge 13 commits into
base: staging
Choose a base branch
from

Conversation

Amantini1997
Copy link
Contributor

This fixes #5311

Description

I created the component that allows to select/deselect visible columns. Note that it updates the table on close (this is to prevent a UI oddity where the list moves right and left as you update it). The settings component will disable the last selected (visible) column, so that at least one column is always visible

I also enhanced the resizable table so that, instead of just receiving the "hiddenColumns" as a prop, it allows the children to update it.

Test cases

Add/Remove columns, change template, change selected models, group by different stuff:

  • the table should remember the columns sizes as they disappear and re-appear;
  • the table should remember the hidden columns, but refresh what is visible on template change.

Base automatically changed from ISSUE_5310 to staging February 14, 2025 16:09
<FormattedMessage id="ticketTable.column.header.title" defaultMessage="title" />
</SortingTableHeader>
<SortingTableHeader name="modelName">
<FormattedMessage id="ticketTable.column.header.federationContainer" defaultMessage="federation / container" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Would there be any drawbacks to using the new COLUMN_NAME_TO_SETTINGS_LABEL from ticketsTable helpers in place of these formatted messages?

Copy link
Contributor Author

@Amantini1997 Amantini1997 Feb 25, 2025

Choose a reason for hiding this comment

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

I did this originally, but I found it harder to read honestly. I will proceed to refactor it and push it, so you can take a look. Lmk then now if you want to keep it as is

Copy link
Contributor

Choose a reason for hiding this comment

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

If you are looking at a template with few columns and then change to a template with many columns the columns will overflow and create a scrollbar. On refresh the columns are resized to fit the screen. Is this how it should be?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have not received direct instructions about this tbh, but I think the idea is to keep track of the columns width and maintain across the templates, which would not be the case. If you were to resize the table every time you change the template. But let's see what @carmenfan thinks about this

Copy link
Member

Choose a reason for hiding this comment

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

My instinct would be it doesn't matter too much either way, by changing the template we're changing context so whether we keep the previous settings or reset everything it's a bit here nor there.

But let me spin up a deployment and have a play

Copy link
Member

Choose a reason for hiding this comment

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

yea i don't think I care, whatever is easiest.

Unrelated note, also I'm not sure if @sanmont3drepo wanted it as a separated issue, but the list of possible columns should include all possibile properties and module properties. Atm I'm only seeing the default properties

Copy link
Member

Choose a reason for hiding this comment

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

and group by should contain all the proeprties currently shown

Again not sure if Santiago wanted them as separate issues so ignore me if that's the case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

He didn't tell me anything about new columns/groups, but knowing him, that will probably be for another issue

Copy link
Member

Choose a reason for hiding this comment

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

ok can you make a note to check with him? it is in the description he wrote and I'm not seeing another issue
image

@carmenfan
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks pretty good to me. The only thing I'm wondering is if there should me a min-width on the table to prevent this from happening with the new ticket button
image

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.

4 participants