-
Notifications
You must be signed in to change notification settings - Fork 57
Add onReceiver to a doc #129
Comments
This will probably require that ONReceiver be generalized such that it doesn't require a |
We also have an opportunity here to use the type system to implement only the events that are relevant for given target. For example selectionchange event only works when installed on the document so it shouldn't be available for regular Element. I've looked at |
One thing I don't really understand is why |
Here is what I have in mind: marad@c50efd3 That's just an MVP of what I was thinking about. I also corrected TodoApp to use this new approach and it seem to work well. Please tell me what do you think about it. I'd probably change the |
I don't understand why Otherwise your approach looks like a significant improvement, once you're happy with it please send a pull request and I'll merge and do a new release. |
Hey @sanity, what is the purpose of this https://github.com/kwebio/kweb-core/blob/master/src/main/kotlin/kweb/html/events/keySpecificKeyup.kt ? I have a feeling that this has to do with ONReceiver/ONImmediateReceiver being |
Since I noticed that it has to do with the |
In this PR I recreated all the events that already were implemented. One thing that is different here is that client would need to specifically import extension functions for them to work. When writing code this is probably not a big deal because IDE would automatically import the function, but merging this is going to break existing builds because they require the imports. Also I was wondering is this approach worth the hustle. This gives a lot of flexibility because you can specifically define which elements get which events, but the truth is - it's a lot of complexity and I've encountered only Please review the code and tell me what you think. |
Ok, after a false start I've released 0.7.14 including your improvements. I made a few changes, including moving the extension functions to the I realize I may have been a bit trigger happy in pushing out the improvements (I only saw this comment after I did the initial release)., but happy to discuss further, particularly think if the complexity trade-off isn't worth it. |
I think that's ok, since you moved the extension functions so the change should be transparent for the users. Unless they set up their IDE to expand the star imports into individual ones. I had the idea to use the DSL so we could for example place I think that by splitting the events and restricting which events we could listen would mean that the maintainer need to keep up with the events "ecosystem" and update the code accordingly, and what we get in return? Nicer API, in theory, but in practice we could also miss some connections (like the I think that it would be better to not split the events and allow installing every listener on every node (like in JS). That would get rid of those extension function monstrosities :) If you agree with this, please let me know. I can prepare another PR which should simplify things a lot. |
Yes, that sounds like the right approach - I'll keep an eye out for the PR, thank you :) |
I'd still like some input on this file: https://github.com/kwebio/kweb-core/blob/master/src/main/kotlin/kweb/html/events/keySpecificKeyup.kt What is the use case for this or why it's there? I can imagine that the user can implement something like this quite easily and I don't quite understand what is the difference between this and regular And with this also the |
Oh, yes - sorry I missed that. The purpose of that is to allow event listeners to be added for specific key presses without having to send every keypress to the server to be checked to see whether it matches the key (which would happen if we used a simple if statement in the event handler). This is kinda specialized, but I found the use case was common enough that it seemed to make sense to support it directly. It's probably not the cleanest part of the code and there is probably a more general way to handle it. Does that answer your question? |
Sure, thanks for clarification :) |
@sanity today I looked at this |
@marad That makes sense to me if you're able to make that change. |
I can, but I think this could be separate issue that I could address in another PR. |
This is done. Thanks! |
There are specific events that can be tracked only on the document, but it's api doesn't allow listening for events.
For example this event only works when installed on the document: https://developer.mozilla.org/en-US/docs/Web/API/Document/selectionchange_event
Here are all events supported by the document: https://developer.mozilla.org/en-US/docs/Web/API/Document#Events
The text was updated successfully, but these errors were encountered: