Fix: event serialization fails for non-DOM-events #79
Conversation
js/src/VueRenderer.js
Outdated
| @@ -77,8 +77,11 @@ function pickSerializable(object, depth=0, max_depth=2) { | |||
| export function eventToObject(event) { | |||
| if (event == null) { | |||
There was a problem hiding this comment.
This if is redundant
js/src/VueRenderer.js
Outdated
| @@ -77,8 +77,11 @@ function pickSerializable(object, depth=0, max_depth=2) { | |||
| export function eventToObject(event) { | |||
| if (event == null) { | |||
| return event; | |||
| } else if(event instanceof Event) { | |||
| return pickSerializable(event); | |||
| } else { | |||
There was a problem hiding this comment.
nit-pick, but else can also be left out
There was a problem hiding this comment.
you mean you prefer
if (event instanceof Event) {
return pickSerializable(event);
}
return event;over
if (event instanceof Event) {
return pickSerializable(event);
} else {
return event;
}right?
I do too, and I do this in Python by default, but I'm not sure it is idiomatic JS.
There was a problem hiding this comment.
yeah, I prefer the first one. I never thought of that being language dependent.
tests/ui/test_events.py
Outdated
| assert box is not None | ||
| # if we pass floats (like 12.5) we get back ints from the | ||
| # event (12 in the case of 12.5), so we cast to int | ||
| cx = int(box["x"] + box["width"] / 2) |
There was a problem hiding this comment.
Not important, but for my context; why do we need to click the center? Isn't an offset of top and left of 1px enough?
There was a problem hiding this comment.
Good point, it seems that not even an offset was needed, just box['x']. My brain was wired to do this due to flakey-tests.
There was a problem hiding this comment.
I now see in your change that even the offset isn't necessary :-)
There was a problem hiding this comment.
GitHub is confused :)
(6 minutes ago is before 5 minutes ago)
maartenbreddels
force-pushed
the
fix_event_serialization
branch
from
f80b2e2
to
5b00f63
Compare
We did not add tests in widgetti#76.
maartenbreddels
force-pushed
the
fix_event_serialization
branch
2 times, most recently
from
78d6a56
to
5974ca1
Compare
If the event object was a string, we would call pickSerializable on it which would convert it to an object, which results in a dict on the Python side.
maartenbreddels
force-pushed
the
fix_event_serialization
branch
from
5974ca1
to
e8cf480
Compare
mariobuikhuizen
changed the title
We did not test the serialization in #76 which introduced a bug (#78). This PR adds tests for both case and fixes #78. Although it does not test example in #78 we know this goes through the same codepath.
Ideally, we also test component event (https://vuejs.org/guide/components/events.html) but we do not support them in templates. Also, without ipyvuetify we do not have a widget that can emit a component event with a non-Event object as argument. We could decide to depend on ipvuetify for the test, or rely on the fact that the custom events in templates share a similar codepath.