Skip to content

Conversation

apinkert
Copy link
Collaborator

For RHCLOUD-40372. A preview can be see in the comment below.

@patternfly-build
Copy link

patternfly-build commented Jul 28, 2025

@aferd
Copy link
Collaborator

aferd commented Jul 31, 2025

lgtm! can we add a cypress test as well, just to keep inline with the rest of the components.

@apinkert
Copy link
Collaborator Author

apinkert commented Aug 4, 2025

@aferd do you know if the cypress component tests are actually being used? I can't get them to run locally (and it doesn't look like the cypress component tests are part of the test_cypress check below) but I don't have an issue running the one cypress e2e test.

Copy link
Contributor

@nicolethoen nicolethoen left a comment

Choose a reason for hiding this comment

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

I am confused about the relationship between this new component and the existing ColumnManagementModal. Can the documentation and file structure of these components be combined?

I think the ColumnManagementModal should be leveraging this ColumnManagement component under the hood. And they should not be separate nav items.

@karelhala
Copy link
Collaborator

@nicolethoen we didn't wanted to remove the ColumnManagementModal implementation, but because it's not really surfaced anywhere we could add another example in the ColumnManagement component with modal example and remove the ColumnManagementModal 🤔 Do you want @apinkert to do it in this PR or seperate one?

@nicolethoen
Copy link
Contributor

I like the idea of the ColumnManagement component being something that can be dropped into a Modal and doing that in our docs in lieu of the preexisting ColumnManagementModal component.

It looks like 4 products are currently using the ColumnManagementModal according to PF analytics reports, so we cannot outright move it, but we could remove it from the docs - or mark it deprecated in the short term.

I wonder if the component name (and relevant props) should use a different term other than ColumnManagement, since this component could be used for any sort of list management - like nav items. I've asked our designers if they have an opinion about the name.

@nicolethoen
Copy link
Contributor

@edonehoo suggested for naming ideas:

  • Display manager / editor
  • List manager
  • View manager
  • Layout manager / Component layout manager (might get confused with our actual Layouts)

Curious about people's opinions. I think I like 'view manager' the best?

@apinkert
Copy link
Collaborator Author

apinkert commented Aug 6, 2025

I like List or View Manager personally, but don't have a strong preference! I think renaming this component makes sense in the context of this PR, and dropping it into the Column Management Modal should be a separate PR.

@apinkert
Copy link
Collaborator Author

apinkert commented Sep 3, 2025

@nicolethoen thanks for the feedback! I've gone ahead and added your suggestions and more specifically changed isShown to isSelected for clarity to represent the checked state of the row

Copy link
Contributor

@nicolethoen nicolethoen left a comment

Choose a reason for hiding this comment

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

@apinkert Looks like you'll just have to regenerate the snapshot tests and i have a couple other tweaks, but then I think we are good to go!

@apinkert
Copy link
Collaborator Author

@nicolethoen Thanks so much! Sorry, I was on PTO the last few days but I just pushed a commit with your suggestions. The spacing looks great 👍

@nicolethoen nicolethoen changed the title Add drag drop column component Feat: add ListManager component group Sep 11, 2025
@nicolethoen nicolethoen merged commit 81f752e into patternfly:main Sep 11, 2025
6 checks passed
@apinkert apinkert deleted the drag-drop-column branch September 11, 2025 13:26
Copy link

🎉 This PR is included in version 6.3.0-prerelease.6 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants