-
Notifications
You must be signed in to change notification settings - Fork 11
feat: lists react to value prop changes in items #750
Conversation
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.
Looking good!
Can you add an automated test for this?
src/components/calcite-pick-list-item/calcite-pick-list-item.tsx
Outdated
Show resolved
Hide resolved
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.
Can we add a test as well?
src/components/calcite-pick-list-item/calcite-pick-list-item.tsx
Outdated
Show resolved
Hide resolved
src/components/calcite-pick-list-item/calcite-pick-list-item.tsx
Outdated
Show resolved
Hide resolved
@driskull can we make the existing event name change in a separate PR? |
Yes, it can be a separate pr for |
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.
Looks good but needs some tests. Could be done in a follow up issue if necessary.
@driskull I added a test. Please take a second look. |
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.
👍
Can you link the issue for renaming the other event to this review?
I don't believe there is an issue for it. Mind creating one? |
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.
Minor comment, good to merge after renaming. 👍
@@ -0,0 +1,57 @@ | |||
<!DOCTYPE html> |
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.
For consistency, the demo filename should be kebab-cased.
Related Issue: #738
Summary
This addresses the bug Kumar raised in which if the value of an item changed after that item was selected, the parent list's Map was not updating its keys. This is due to the mutationObserver not reacting to attribute changes - which is necessary since attribute changes are too frequent.
So we added a watcher to the value prop and emit a custom event when it changes, that the list can listen to and respond accordingly.