search

AlaskaAirlines / auro-radio

Custom input element of type radio generally used in radio groups—collections of radio buttons describing a set of related options
https://auro.alaskaair.com/components/auro/radio
Apache License 2.0
1 stars 6 forks source link

Component event strategy #26

Open geoffrich opened 3 years ago

geoffrich commented 3 years ago

Is your feature request related to a problem? Please describe.

It's tricky listening to component events in React. Sometimes inline handlers work (e.g. onInput), and sometimes you have to have to attach event listeners using a ref. Because of React's synthetic event system, you can only use inline handlers if the event is a native DOM event. If it's a custom event, you have to use a ref.

auro-radio is tricky. The input event emitted from the component is from the inner input. This is because input is a composed event -- it propagates across shadow DOM boundaries. Because it's native, you can listen to it using onInput. change is different. The native change event is not composed, so we re-dispatch it as a custom event. Because of this, you can't listen to the change event using onChange in React and you have to use a ref.

This is confusing, and it's not clear to the consumer that there's a difference.

Describe the solution you'd like

We should distinguish custom component events from native ones so that it's clear there's a difference.

Instead of dispatching a change event, we dispatch an auro-change event instead. This makes it clear that this is a custom event instead of a native event. It's also similar to how other component libraries name their events.

I'm still thinking through whether we'd want to name all component events like this. The escaping input event is really a leaky abstraction -- should the consumer care how the component is implemented under the hood? Or do we tell them what events to listen to? For instance, Shoelace makes heavy usage of custom events.

Either way, by prefixing our custom events, we make it clear to consumers that they are custom events. If the event begins with auro-, they should assume that they need to use a React ref to set up event listeners instead of an inline listener.

Some related thoughts from Westbrook: composed: true considered harmful?.

Describe alternatives you've considered

Instead of changing component behavior, we simply document the difference between the events and what it means for React.

blackfalcon commented 3 years ago

Meeting to review this feature request has been scheduled.

blackfalcon commented 3 years ago

100% agree with the idea that we are creating custom elements and by extension creating custom events are expected but embraced in this community.

A couple of things.

  1. This element, as well as auro-checkbox, are up for massive functional and feature updates. It would be best to kick this can down the road and coordinate a MAJOR release that addresses this concern.
  2. See https://github.com/AlaskaAirlines/WC-Generator/issues/155
blackfalcon commented 3 years ago

Either way, by prefixing our custom events, we make it clear to consumers that they are custom events. If the event begins with auro-, they should assume that they need to use a React ref to set up event listeners instead of an inline listener.

I really like the clarity of this versus trying to get events to appear as native HTML, which we are not using.

In regard to the React Ref, I am hoping we can obscure that with the future migration to Lit2.0 and React wrappers.

blackfalcon commented 3 years ago

@geoffrich so that we are on the same page, what is it in THIS repo that you specifically want to see updated? Assuming that we are talking about the following lines? https://github.com/AlaskaAirlines/auro-radio/blob/master/src/auro-radio.js#L60-L82

While we agree conceptually, this issue does not have an expectation and/or exit criteria.

geoffrich commented 3 years ago

The existing custom events are renamed and become the preferred way to interact with the component.

old event new event
change auro-change
toggleSelected auro-input
focusSelected auro-focus

This will be a breaking change.

blackfalcon commented 2 years ago

This issue is bound to https://github.com/AlaskaAirlines/WC-Generator/issues/224

blackfalcon commented 2 years ago

Had a conversation with @geoffrich and while there is an opportunity to update this API, it's not a critical issue to address. Teams who have consumed this element in React have engineered wrappers that address this shortcoming. There is also the opportunity that updates in React will nullify the need for this as well ¯_(ツ)_/¯

The concept of hijacking native events has been a decision that was made when this all started... that being said, had we not done that, there are things that would have been a lot easier to do as well (auro-input and the webkit cursor issue).

While we agree that there is something that can be done here, I'd like to take into consideration all the other issues around events and reactive code and propose that there is a concentrated effort to update all the form elements with new form support in the browser pertaining to web components.

blackfalcon commented 1 year ago

I want to get this back into the rotation so that we can discuss this update and how it related to the larger goal of more reactive form elements with the new formData API opportunities.