-
Notifications
You must be signed in to change notification settings - Fork 426
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
Move defaultEventNames into Schema #662
base: main
Are you sure you want to change the base?
Conversation
This allows applications to extend the mapping with support for custom elements. Closes hotwired#660
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few thoughts, I think this would be an awesome feature and helps for mixed adoption of Web Components.
@@ -29,6 +30,22 @@ export const defaultSchema: Schema = { | |||
// [0-9] | |||
...objectFromEntries("0123456789".split("").map((n) => [n, n])), | |||
}, | |||
defaultEventNames: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
defaultEventNames: { | |
defaultEventMappings: { |
Since we use the term mappings already, might be nice to align with that naming?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Obsoleted by 2ddac5f
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ended up dropping 2ddac5f so this is relevant again. I'm open to renaming it.
This probably needs docs also |
Co-authored-by: LB (Ben Johnston) <[email protected]>
Definitely up for adding docs. Can I interpret your review as tacit support for this implementation vs. #661? |
I'm not on the core team - but I love the idea of this feature so it gets my vote 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2ddac5f
to
0af60e5
Compare
0af60e5
to
4ecb43d
Compare
This now takes into account the feedback on #661 as well, setting on an I opted for the plural form of the name and passing an object because it felt nicer to write this: app.registerDefaultEventNames({
'sl-button': 'click',
'sl-select': 'change',
'sl-input': 'input',
// etc.
}) vs this: app.registerDefaultEventName('sl-button', 'click')
app.registerDefaultEventName('sl-select', 'change')
app.registerdDefaultEventName('sl-input', 'click')
// etc. ... but I'm open to changing it back to Other concernsThere's a limitation (also covered in the docs) where this API won't work when calling Application.start(document.body, {
...defaultSchema,
defaultEventNames: {
...defaultSchema.defaultEventNames,
'sl-button': 'click',
// etc.
}
}) |
This allows applications to extend the mapping with support for custom elements.
Closes #660
See also: this comment #661 (comment)