Skip to content
This repository has been archived by the owner on Jun 29, 2023. It is now read-only.

feat: lists react to value prop changes in items #750

Merged
merged 6 commits into from
Jan 17, 2020

Conversation

pr3tori4n
Copy link
Contributor

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.

@pr3tori4n pr3tori4n requested a review from a team as a code owner January 10, 2020 20:19
@pr3tori4n pr3tori4n self-assigned this Jan 10, 2020
@pr3tori4n pr3tori4n added the bug Something isn't working label Jan 10, 2020
Copy link
Member

@jcfranco jcfranco left a 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?

Copy link
Member

@driskull driskull left a 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?

@pr3tori4n
Copy link
Contributor Author

@driskull can we make the existing event name change in a separate PR?

@driskull
Copy link
Member

Yes, it can be a separate pr for calciteListItemPropsUpdated but its a breaking change so we can either do it now or deprecate later.

Copy link
Member

@driskull driskull left a 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.

@pr3tori4n
Copy link
Contributor Author

@driskull I added a test. Please take a second look.

Copy link
Member

@driskull driskull left a 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?

@pr3tori4n
Copy link
Contributor Author

👍
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?

Copy link
Member

@jcfranco jcfranco left a 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>
Copy link
Member

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.

@driskull
Copy link
Member

#773

@pr3tori4n pr3tori4n changed the title lists react to value prop changes in items feat: lists react to value prop changes in items Jan 17, 2020
@pr3tori4n pr3tori4n merged commit 837d85d into master Jan 17, 2020
@pr3tori4n pr3tori4n deleted the hrobbins/value-list-change-value-738 branch January 17, 2020 23:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants