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

Dynamic collections #559

Closed
wants to merge 13 commits into from
Closed

Dynamic collections #559

wants to merge 13 commits into from

Conversation

orlinmalkja
Copy link
Contributor

This PR aims to accomplish the following goals:

  • Open a new modal when clicking Add Button
  • Create the Tree View of the currently opened panels and show it in the Modal
  • Add a new panel dynamically when either entering a collection url or clicking at a tree item
  • Showing an error message when the user has not provided a way to open a new collection dynamically

@orlinmalkja orlinmalkja self-assigned this Jan 22, 2025
@orlinmalkja
Copy link
Contributor Author

Please continue reviewing the parts which are not related with linting issues. Integrating Es lint with VSCode has not worked yet for me...

Copy link
Collaborator

@paulpestov paulpestov left a comment

Choose a reason for hiding this comment

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

The structure you chose to render the tree is not very flexible. In your code the components are not very reusable. They rather follow a strict hierarchy. Imagine we will implement later the lazy loading feature that would be required on manifest and items level both. YOu would need to implement it twice because you have sepcial components for manifest and item sub trees.

A better approach would be general tree node components that can render recursively other tree node comps as children. Each node component can toggle it's children display if there are any. And later you just add the lazy loading function onto the children within that node component.

Our idea of having collections in collections would also come closer.

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.

2 participants