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.

Sign up for GitHub

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

Jump to bottom

✨ integrate column management feature into apps table #1767

Merged
merged 4 commits into from
Mar 27, 2024
Merged

✨ integrate column management feature into apps table #1767

merged 4 commits into from
Mar 27, 2024

Conversation

ibolton336
Copy link
Member

@ibolton336 ibolton336 commented Mar 13, 2024 •
edited
Loading

Resolves #1765

@ibolton336
Copy link
Member Author

#1765

@ibolton336 ibolton336 requested review from sjd78 and rszwajko March 14, 2024 17:44
@sjd78 sjd78 linked an issue Mar 14, 2024 that may be closed by this pull request
Integrate col management to applications table #1765
Closed
rszwajko
rszwajko reviewed Mar 18, 2024
View reviewed changes
Copy link
Collaborator

@rszwajko rszwajko left a comment

Choose a reason for hiding this comment

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

Some comments below (based on experience gathered from kubev2v/forklift-console-plugin#64 and follow-ups)

client/src/app/hooks/table-controls/column/useColumnState.ts Outdated Show resolved Hide resolved
client/src/app/hooks/table-controls/useTableControlProps.ts Outdated Show resolved Hide resolved
client/src/app/pages/applications/applications-table/applications-table.tsx Outdated Show resolved Hide resolved
client/src/app/pages/applications/applications-table/applications-table.tsx Outdated Show resolved Hide resolved
@mturley mturley mentioned this pull request Mar 18, 2024
Open
@ibolton336 ibolton336 force-pushed the manage-columns-modal branch 2 times, most recently from 316bb70 to 076f9a2 Compare March 21, 2024 14:43
@ibolton336
Copy link
Member Author

Screen.Recording.2024-03-21.at.10.43.45.AM.mov

@ibolton336 ibolton336 marked this pull request as ready for review March 21, 2024 14:45
@ibolton336 ibolton336 force-pushed the manage-columns-modal branch from 076f9a2 to d10dbac Compare March 21, 2024 14:45
@ibolton336 ibolton336 changed the title WIP start integrating column name management tool into apps table ✨ integrate column management feature into apps table Mar 21, 2024
@ibolton336 ibolton336 closed this Mar 22, 2024
@ibolton336 ibolton336 reopened this Mar 22, 2024
@ibolton336 ibolton336 force-pushed the manage-columns-modal branch 3 times, most recently from 5ee0e04 to 139b48b Compare March 22, 2024 19:14
sjd78
sjd78 approved these changes Mar 25, 2024
View reviewed changes
Copy link
Member

@sjd78 sjd78 left a comment

Choose a reason for hiding this comment

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

Overall the change LGTM. A few comments, and only 1 console.log that should be taken out.

client/src/app/hooks/table-controls/useLocalTableControls.ts Outdated Show resolved Hide resolved
client/src/app/pages/applications/applications-table/applications-table.tsx
<Th {...getThProps({ columnKey: "tags" })} width={10} />
<Th {...getThProps({ columnKey: "effort" })} width={10} />
{/* {columnState.columns. isVisible then render this column} */}
{getColumnVisibility("name") && (
Copy link
Member

Choose a reason for hiding this comment

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

I don't mind this approach.

The visibility prop exists and is used in the PF docs to use the css breakpoints to show/hide columns. Fun fact is that prop is not in any of the generated docs and it needs to be tracked down in sources. I tried that and got lost pretty quick. Probably isn't worth trying to use visibility for the user based show/hide column support.

client/src/app/hooks/table-controls/column/useColumnState.ts
const [columns, setColumns] = useState<ColumnState<TColumnKey>[]>(
args.initialColumns
);
return { columns, setColumns };
Copy link
Member

Choose a reason for hiding this comment

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

And this is where you'd have to add column visibility persistence past the component state level that is used now. I predict there will be a QE ticket filed in less than 10 minutes of app testing say that the column visibility doesn't persist page page refreshes.

A chat before testing would help. Maybe even loop docs in to level set expectations as to how long the column management state lives.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added local storage support for the column names.

sjd78
sjd78 requested changes Mar 25, 2024
View reviewed changes
Copy link
Member

@sjd78 sjd78 left a comment

Choose a reason for hiding this comment

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

Now that I'm running it to play around, I have 2 comments, one of which needs to be resolved for sure:

  1. The column name in the list is not center aligned (needs to be fixed)
  2. I highly suggest that the Name column is setup to be required. It can show up on the list, and the checkbox checked, but the checkbox being disabled.

Screen shot from FF:
image

@ibolton336 ibolton336 force-pushed the manage-columns-modal branch from fee201f to e2a0503 Compare March 25, 2024 19:55
@ibolton336
Copy link
Member Author

Screenshot 2024-03-25 at 5 09 46 PM Updated to match pr suggestions / UXD suggestions.

sjd78
sjd78 reviewed Mar 26, 2024
View reviewed changes
Copy link
Member

@sjd78 sjd78 left a comment

Choose a reason for hiding this comment

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

Looks really good. The only highly recommended change would be for the i18next strings in the ManageColumnsToolbar.

client/src/app/pages/applications/applications-table/components/manage-columns-modal.tsx Show resolved Hide resolved
client/src/app/pages/applications/applications-table/components/manage-columns-toolbar.tsx Outdated Show resolved Hide resolved
@ibolton336
PR suggestions
797d152 
Signed-off-by: Ian Bolton <[email protected]>
@ibolton336
PR suggestions
7d245ed 
Signed-off-by: Ian Bolton <[email protected]>
@ibolton336 ibolton336 force-pushed the manage-columns-modal branch from 463ee9a to 7d245ed Compare March 26, 2024 17:57
@ibolton336 ibolton336 requested review from rszwajko and sjd78 March 26, 2024 17:58
rszwajko
rszwajko approved these changes Mar 27, 2024
View reviewed changes
Copy link
Collaborator

@rszwajko rszwajko left a comment

Choose a reason for hiding this comment

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

Looks OK to me. Any open points can be addressed in follow-ups.

sjd78
sjd78 approved these changes Mar 27, 2024
View reviewed changes
Copy link
Member

@sjd78 sjd78 left a comment

Choose a reason for hiding this comment

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

LGTM

@ibolton336 ibolton336 merged commit c2361a9 into konveyor:main Mar 27, 2024
6 checks passed
This was referenced May 8, 2024
Merged
Merged
This was referenced Jun 5, 2024
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JustinXHale JustinXHale JustinXHale left review comments

@mturley mturley mturley left review comments

@rszwajko rszwajko rszwajko approved these changes

@sjd78 sjd78 sjd78 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Integrate col management to applications table
5 participants
@ibolton336 @mturley @sjd78 @rszwajko @JustinXHale