Open wesruv opened 3 years ago
I don't see how that PR addressed the issue, which is still occuring. Reopening.
I'd like to suggest a different approach.
Composed events might not be the way we want to go here. Other design systems use non composed events. For example these events on <vaadin-upload>
or these events on <mwc-snackbar>
By counter-example, shoelace does tend to compose events: https://github.com/shoelace-style/shoelace/blob/9c31d148feff0c40b6d3407d85b4c4872e0832f3/src/internal/event.ts#L11
as does FAST:
So for analytics events, I propose that we in fact remove analytics events from PFE, subclass elements in <rh-*>
and implement composed analytics events there.
Proposal doc: https://docs.google.com/document/d/1tSAV0aysOrMl3XupUuY7oV50Cxlu7AcxZdV-gtowDzg/edit#heading=h.mx5h1h2oj4l2
A nice counter-point by the author of the linked article
https://twitter.com/WestbrookJ/status/1471830306617450498?s=20
Arg, well apparently that PR did address this issue, as it made all events composed
, but now we're talking about not using composed events.
So that let's hash this out a bit @bennypowers
The issue was:
So if we have something like this:
body
- parent component
- shadow-root
- child component
- shadow-root
- button
A 'non-composed' event from button could be written to emit from the child component tag, making it out to parent component, but not to the body. Since the analytics script is listening in the body, and we lose the ability to reliably track UX interactions.
In the case of pfe-content-set
it'd make sense for it to capture events from pfe-tabs
and pfe-accordion
. We know those are expected children components, and should send out a generic opened/closed events.
But we have a few components that use 'light DOM as data' and don't know what components will be in it's shadow root (e.g. the second version pfe-navigation that's in the branch https://github.com/patternfly/patternfly-elements/tree/CPFED-3689-new-navigation-epic/elements/pfe-navigation) and cp-documentation from cp-elements.
Both are using 'light DOM as data' to get style encapsulation so the parent site can't accidentally effect the appearance of important parts of it's UI that involve content that the site (not the component) should provide.
So if I understand the composed: true
article, by setting composed:true
universally we're using more performance budget than we may want to, but if we turn switch back we reintroduce significant issues tracking UX interactions with analytics; and I don't see a straightforward fix to this issue.
Hopefully this makes sense, glad I mistakenly reopened this, if it doesn't make sense maybe we can do a call.
Hey yeah I think your read on this is good. In the twitter thread I linked above quite a number of wc movers-and-shakers replied and the general consensus is that "UI events" should compose but "app events" should not, where "UI events" are things like click
, change
, opened
, select
, etc and "app events" are things like user-added
, product-liked
etc - app events are relevant to app builders, less so to PFE.
I will review our new events and make sure they compose.
Description of the issue
Working with some analytics folks they mentioned they couldn't get the show/hide events from the tabs on redhat.com's contact page. The tabs are inside of a pfe-content-set and it looks like the events from the child components don't make it outside of pfe-content-set.
Impacted component(s)
Steps to reproduce
document.addEventListener('pfe-tabs:shown-tab', function(event) {console.log('lightDomListener', event)});
document.querySelector('pfe-content-set').shadowRoot.addEventListener('pfe-tabs:shown-tab', function(event) {console.log('shadowDomListener', event)});
Expected behavior
I'm thinking pfe-content-set could boost the child events, and it might be nice if it pushed out a generic event (so a site dev wouldn't have to know which state the content set was in).