Skip to content

Conversation

@muan
Copy link
Contributor

@muan muan commented Apr 4, 2019

@muan muan requested a review from a team April 4, 2019 23:26
@muan muan changed the title Update event, docs, and tests. Update event, docs, and tests (breaking changes) Apr 4, 2019
@muan muan changed the title Update event, docs, and tests (breaking changes) Update event, docs, and tests (breaking change) Apr 4, 2019
this.dispatchEvent(
new CustomEvent('tab-container-change', {
bubbles: true,
detail: {relatedTarget: panel}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we normally use this pattern to denote the element that changed? Could we just use target and currentTarget rather then relatedTarget?

Something like:

diff --git a/index.js b/index.js
index e6695c4..494c79a 100644
--- a/index.js
+++ b/index.js
@@ -61,12 +61,7 @@ class TabContainerElement extends HTMLElement {
     tab.focus()
     panel.hidden = false
 
-    this.dispatchEvent(
-      new CustomEvent('tab-container-change', {
-        bubbles: true,
-        detail: {relatedTarget: panel}
-      })
-    )
+    panel.dispatchEvent(new CustomEvent('tab-container-change', {bubbles: true}))
   }
 }
 
diff --git a/test/test.js b/test/test.js
index 468535f..83f05c1 100644
--- a/test/test.js
+++ b/test/test.js
@@ -44,7 +44,7 @@ describe('tab-container', function() {
       let counter = 0
       tabContainer.addEventListener('tab-container-change', event => {
         counter++
-        assert.equal(event.detail.relatedTarget, panels[1])
+        assert.equal(event.target, panels[1])
       })
 
       tabs[1].click()

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dig the relatedTarget approach. If tab-container-change keeps being fired from the <tab-container> element, then it's easier to detect in delegated handlers whether the current tab-container-change came from the current <tab-container> element or from a nested one:

on('tab-container-change', '.my-container', function(event) {
  if (event.target !== event.currentTarget) return
  // ...
})

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Talked to @koddsson F2F- for posterity, to add to @mislav's points, also:

  • This ensures that event alway has the two things we care about no matter where/how it was caught, without needing extra queries. target is always <tab-container> and event.detail.relatedTarget is always the panel. (For comparison, previously if you listen for the event bubbling up to document, we will need a query to find the <tab-container>.
  • Custom Element always fires Custom Event from/on itself, instead of from its non-custom children (unexpected).
  • These guidelines are flexible enough to be applicable to all our elements with/without the need for relatedTarget or bubbles.

this.dispatchEvent(
new CustomEvent('tab-container-change', {
bubbles: true,
detail: {relatedTarget: panel}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dig the relatedTarget approach. If tab-container-change keeps being fired from the <tab-container> element, then it's easier to detect in delegated handlers whether the current tab-container-change came from the current <tab-container> element or from a nested one:

on('tab-container-change', '.my-container', function(event) {
  if (event.target !== event.currentTarget) return
  // ...
})

Copy link
Contributor

@dgraham dgraham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great for a 0.2.0 release.

@muan muan merged commit f8ec0f2 into master Apr 5, 2019
@muan muan deleted the event branch April 5, 2019 21:19
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.

5 participants