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
fix(navigation): sync state with newly added nav items #156
fix(navigation): sync state with newly added nav items #156
Conversation
👋 @ashleyryan,
Thank you, 🤖 Clarity Release Bot |
c9536a6
to
41efc73
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. If you find time some parts of the code of the navigation could be moved to controllers.
Worth checking if cds-navigation-group could reuse
https://github.com/vmware-clarity/core/blob/main/projects/core/src/internal/controllers/key-navigation-list.controller.ts
Yeah I had the same thought when I was looking at the code. Might be a good exercise to get some more lit experience. |
7b81dbe
to
a316a59
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the chore commit message should be reworded for correctness, but this otherwise seems okay to me. I don't know web components well, but I don't see anything bad.
fixes vmware-clarity#155 vmware-clarity#82 - previously only nav items in the initial render were synced with the top level expanded state - add onslotchange to check for that - move tabindex logic to slot change handler as well - update unit tests to check syncing after component updates
a316a59
to
fcbd0cd
Compare
Commit message fixed. The slotChange event notifies you if content has been added/removed from the slot. So instead of only syncing the props, and setting tabIndex on firstUpdate or when the component updates, it will do it everytime a new nav item is added (or removed, which is unnecessary, but not really a problem) |
🎉 This PR is included in version 6.1.2 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
Hi there 👋, this is an automated message. To help Clarity keep track of discussions, we automatically lock closed PRs after 14 days. Please look for another open issue or open a new issue with updated details and reference this one as necessary. |
fixes #155 #82
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Previously, expanded state was only shared with nav items on the initial render, or when the parent navigation component had updates. Asynchronously added nav items were treated as collapses, instead of having their parent state synced
Issue Number: #155 #82
What is the new behavior?
the slotchange event was added to the slot to detect when new items are added
Does this PR introduce a breaking change?
Other information