Skip to content

Fix custom change event listener #2

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

Merged
merged 3 commits into from
Mar 6, 2019
Merged

Fix custom change event listener #2

merged 3 commits into from
Mar 6, 2019

Conversation

muan
Copy link
Contributor

@muan muan commented Mar 6, 2019

Anyone can fire a custom change event. This ensures that it doesn't throw when event.detail is null. cc @latentflip

cc @github/web-systems

@latentflip
Copy link

Nice 👍 I think we also need to make a similar change here:

check-all/check-all.js

Lines 65 to 71 in 3432728

function onCheckAllItem(event: Event): void {
if (event instanceof CustomEvent) {
const {relatedTarget} = event.detail
if (relatedTarget.hasAttribute('data-check-all') || relatedTarget.hasAttribute('data-check-all-item')) {
return
}
}

   function onCheckAllItem(event: Event): void {
-    if (event instanceof CustomEvent) {
+    if (event instanceof CustomEvent && event.detail) {
       const {relatedTarget} = event.detail
-      if (relatedTarget.hasAttribute('data-check-all') || relatedTarget.hasAttribute('data-check-all-item')) {
+      if (relatedTarget && relatedTarget.hasAttribute('data-check-all') || relatedTarget.hasAttribute('data-check-all-item')) {

         return
       }
     }

@latentflip
Copy link

@koddsson @muan what do you think about my comment here: #2 (comment)

@koddsson
Copy link
Contributor

koddsson commented Mar 6, 2019

@koddsson @muan what do you think about my comment here: #2 (comment)

Looks good to me. I kind of wish that we didn't have to check all attributes to see if they are present but I think we probably need to if we are listening to the change event, huh?

@muan muan merged commit f0fb8f6 into master Mar 6, 2019
@muan muan deleted the detail branch March 6, 2019 16:00
@muan
Copy link
Contributor Author

muan commented Mar 6, 2019

Release out 0.2.1

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.

3 participants