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

feat(@clayui/nav): add new composition to Vertical Nav with collection API and adds new expandedKeys API #5526

Merged
merged 8 commits into from
May 15, 2023

Conversation

matuzalemsteles
Copy link
Member

@matuzalemsteles matuzalemsteles commented May 12, 2023

Close #5515

This PR makes a lot of changes, first we are rewriting the VerticalNav component to use the Collection API and exposing a new public <VerticalNav.Item /> component.

We are also adding a new expandedKeys API with the pattern of controlled and uncontrolled, this makes it easier to manage this state for items in the tree like implementing expand all or collapse.

We are also moving the implementation of all this to the @clayui/core package and the @clayui/nav package just keeps backwards compatibility with the old version of the component and possibility to use the new version when declaring the new composition.

With the Collection implementation I needed to rewrite the component and this brings a new composition with the benefits of the Collection pattern.

<VerticalNav items={items}>
    {(item) => (
        <VerticalNav.Item active={item.active} items={item.items}>
            {item.label}
        </VerticalNav.Item>
    )}
</VerticalNav>

This brings new possibilities to the component as it is also data agnostic and does not depend on a data structure to force the product to follow. As I said, we still maintain compatibility with the previous version, so it's easy to migrate to use the new composition and the new features that Collection brings, also the expandedKeys API will only take effect with the new composition because it depends on which keys in the tree is unique. Consuming the component directly from the @clayui/core package will only support the new composition.

ToDo

  • Update docs

…n API

This commit makes a lot of changes, first we are rewriting the VerticalNav component to use the Collection API and exposing a new public VerticalNav.Item component.

We are also adding a new `expandedKeys` API with the pattern of controlled and uncontrolled, this makes it easier to manage this state for items in the tree like implementing expand all or collapse.

We are also moving the implementation of all this to the `@clayui/core` package and the `@clayui/nav` package just keeps backwards compatibility with the old version of the component and possibility to use the new version when declaring the new composition.
@matuzalemsteles
Copy link
Member Author

cc @ethib137 I think this will help you implement the collpase mechanism and I added an example in the storybook using the new expandedKeys API.

@ethib137
Copy link
Member

Thanks @matuzalemsteles ! What do you think about adding expanded as a prop to navItem so that expanded can be totally controlled? I think that might be even simpler and more flexible, since to get all the keys for the expanded keys api I would still have to loop through all nested items. If I'm doing that, I might as well just set expanded myself directly on the item. What do you think?

@matuzalemsteles
Copy link
Member Author

What do you think about adding expanded as a prop to navItem so that expanded can be totally controlled? I think that might be even simpler and more flexible, since to get all the keys for the expanded keys api I would still have to loop through all nested items. If I'm doing that, I might as well just set expanded myself directly on the item. What do you think?

@ethib137I think this would cause more complexity, i don't see this being simpler for your case, for example if you want to expand all items you would traverse the tree when you need to expand to collect all expanded items and when close you just sets the state of expandedKeys to new Set() with an empty value. You can do this when making the filter to avoid going through more than once.

@ethib137
Copy link
Member

Okay, sounds good. I'll try it out that way then. Thanks @matuzalemsteles !

@matuzalemsteles matuzalemsteles merged commit 3efa915 into liferay:master May 15, 2023
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.

Is there any way to open all or close all panels of the Vertical Nav?
2 participants