-
Notifications
You must be signed in to change notification settings - Fork 63
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
Discussion: consider moving from event handler properties to events #170
Comments
We have not encountered the usecases you mention, but they certainly seem valid. The What is most important for me, is a simple to understand API with as much Typescript support as possible. FocusIn/FocusOut could also solve some issues in our application, so this could also benefit us. Do other frameworks already have a notation for this that we can borrow? If not, a notation like the following would probably be nice (and backwards compatible), what do you think: h('button', {
tabIndex: -1,
eventListeners: {
click: (evt) => { evt.preventDefault(); doStuff(); }
},
captureEventListeners: {
focusIn: handleFocusIn
}
}, ['Click me']) |
Agreed! Maquette has a really nice and simple API worth preserving.
That could work, but would it be possible to extend the existing notation and change the eventing implementation to use event listeners? If so, the If this is not backwards-compatible, could you share some details on what would be impacted? Is it mostly the APIs that deal with
I'll have to do some digging see what other framework conventions for events are out there. |
right now, maquette actually uses the attribute/property Using |
We actually have pieces of code that look like this, that would be more readable when eventListeners was a separate object, although most vnodes are less complex. {
key: component,
id,
'data-test': testId,
classes: {
'has-hint': !!hint,
'is-disabled': isDisabled(),
'is-hoveringEnabled': appState.isDesktop,
'is-semiActive': state === 'semiActive',
'is-active': state === 'active',
'is-inProgress': inProgress(),
'is-done': done,
'is-error': state === 'error'
},
'data-shortcutsKeys': shortcutsKeys,
onclick: handleButtonClick,
onkeydown: handleButtonKeyDown,
afterCreate: handleButtonAfterCreate,
afterUpdate: registerButton,
afterRemoved,
onfocus: onFocus,
onblur: onBlur,
tabIndex: focusable === false ? -1 : undefined
} |
Right. I was more curious about use cases to consider for the eventing system refactor. It doesn't seem like the internals of
That sounds promising! 😄
I think this would be a sensible plan to move forward. Would the separate event objects be gone in the major release you proposed? FWIW, it does make it bit awkward with existing code since there's no clear distinction of one vs the other unless you know the implementation details the following example assumes event handling code is updated in phases and ended up with a mixed approach {
// ...
onclick: handleClick,
eventListeners: { focusin: handleFocusIn },
captureEventListeners: { click: handleClickCapture }
} vs {
// ...
onclick: handleClick,
onfocusin: handleFocusIn,
onclickcapture: handleClickCapture
} |
Sorry for the late reply. The separate object may be a little too complex for simple cases, so lets forget that idea for now. We could also consider using camelCase properties and make the non camelcase ones obsolete. The camelCase properties would then use addEventListener, while the noncamelcase ones uses the legacy behavior. {
onclick: handleLegacyClick,
onClick: handleClick,
onClickDuringCapture: handleClickDuringCapture
} |
I really dig this proposal. It makes it easy to flip to the new eventing system and keeps the API pretty much the same. It may be easy to miss if there's a mix of legacy and updated event handler names, but may not be a big deal since it's straightforward to switch to the new way. Sidebar, is |
If we mark the old lowercase eventhandlers obsolete, most IDE's will add a strikethrough. This would minimize making mistakes I hope. DuringCapture was just a suggestion to make the code less cryptical and more readable. We could also consider |
Good point on TS marking deprecated props. About the capture event notation, whatever you decide will be great. I was merely suggesting a convention I've seen around in |
We have not found the time in our planning yet to implement this, but the need for this is becoming clearer, as custom elements are very promising. |
Thanks for the status update. It is very exciting to see that you are planning to add support for custom element events.
I'm not sure I follow. My understanding is that custom event names can be anything, so expecting one casing vs another may not work for all cases (ba dum tss 🥁). For example, Stencil's doc has a snippet with camelCase whereas Lit's has one with kebab-case.
Maybe a tad less magical compared to the current event listener story, but I see working regardless of event name casing. |
Today we released maquette 4.0 with support for |
@johan-gorter This is a wonderful addition, thanks! |
Maquette's eventing has been working great for us, but we've come across two use cases that have issues with the existing implementation:
onfocusin
/onfocusout
– Some browsers (Chrome, Firefox, Edge) no longer seem to have event handler properties for these events (test case). For these particular events, we can't useblur
/focus
as they don't bubble. Note that these are the ones that have come up in our development, but there could be others.Changing maquette's event interceptor to use events instead of event handler properties should help with these cases.
I look forward to your thoughts on this and if you think it's something worth doing, I'd be more than happy to help out in any way that I can.
The text was updated successfully, but these errors were encountered: