Skip to content
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

Add event handling support to vdom-extension #5670

Merged
merged 13 commits into from Apr 5, 2019

Conversation

@gnestor
Copy link
Contributor

@gnestor gnestor commented Nov 25, 2018

This adds event handler support to vdom-extension.

See https://github.com/gnestor/jupyterlab/tree/vdom-events-demo/packages/vdom-extension for more details and a binder demo.

To do:

@gnestor gnestor force-pushed the vdom-events branch 3 times, most recently from b61e2c1 to 64ef507 Nov 29, 2018
@ellisonbg ellisonbg added this to the 1.0 milestone Nov 29, 2018
@gnestor gnestor changed the title [WIP] Add event handling support to vdom-extension Add event handling support to vdom-extension Dec 3, 2018
@gnestor gnestor force-pushed the vdom-events branch 3 times, most recently from 0e8bf2a to 32870ab Jan 10, 2019
@gnestor
Copy link
Contributor Author

@gnestor gnestor commented Jan 10, 2019

I can confirm that this is working with the latest release of vdom (0.6) and the latest release of @nteract/transform-vdom (3.0.4-alpha.0) so this is ready to merge 👍

@gnestor
Copy link
Contributor Author

@gnestor gnestor commented Feb 6, 2019

The binder demo was broken, fixed it 👍

Copy link
Member

@ian-r-rose ian-r-rose left a comment

This is awesome @gnestor, I love that demo notebook. Not all of the events seemed to be working (or, at least, the JSON-serialized data was empty). In particular, I couldn't get the image, media, or selection events to work. Do you see the same thing?

packages/vdom-extension/src/index.tsx Outdated Show resolved Hide resolved
packages/vdom-extension/src/index.tsx Outdated Show resolved Hide resolved
packages/vdom-extension/src/index.tsx Outdated Show resolved Hide resolved
packages/vdom-extension/src/index.tsx Outdated Show resolved Hide resolved
@gnestor
Copy link
Contributor Author

@gnestor gnestor commented Feb 7, 2019

@ian-r-rose Regarding the empty event payloads, that's intentional per the vdom event spec. This spec intentionally omits properties that are not easily JSON serializable and is largely based on React's SyntheticEvent.

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Feb 7, 2019

Ah, got it. Thanks for the info.

@gnestor
Copy link
Contributor Author

@gnestor gnestor commented Feb 7, 2019

Are any of these failed checks legitimate?

@ellisonbg
Copy link
Contributor

@ellisonbg ellisonbg commented Feb 25, 2019

Hmm, I built this locally, installed vdom 0.6 but it doesn't seem to work. I see this upon running the first cell:

screen shot 2019-02-25 at 9 49 12 am

Any ideas?

notebooks: INotebookTracker,
restorer: ILayoutRestorer
) => {
const tracker = new InstanceTracker<MimeDocument>({
Copy link
Contributor Author

@gnestor gnestor Mar 12, 2019

@ian-r-rose I observed that other non-extension packages (e.g. htmlviewer) export a tracker instance. Should this vdom package export the tracker instance or can vdom-extension create one itself?

Copy link
Member

@ian-r-rose ian-r-rose Apr 1, 2019

Usually the base package defines the tracker token, and then the extension provides it. This is so that other extensions can import the token without including the *-extension package.

Copy link
Member

@ian-r-rose ian-r-rose Apr 1, 2019

It's not required, however. If you want to do it here, that's cool, but I think it can also be done in a follow-up.

Copy link
Contributor Author

@gnestor gnestor Apr 1, 2019

Can you take a look at 6d89b1c. I used htmlviewer and htmlviewer-extension as reference.

@gnestor
Copy link
Contributor Author

@gnestor gnestor commented Mar 12, 2019

I split the vdom-extension packages into @jupyterlab/vdom and @jupyterlab/vdom-extension to be consistent with other non-mimerender extensions.

Should we call the widget package @jupyterlab/vdom or @jupyterlab/vdomviewer?

Copy link
Member

@ian-r-rose ian-r-rose left a comment

This looks great @gnestor, thanks for your patience in going through the several cycles on this!

There is now a merge conflict (my fault in #6057, I believe). I can rebase if you like, or can leave it to you.

We can also allow the vdom extension to provide the instance tracker, so that other extensions can interact with vdom documents. I think that can be for a follow-up, however.

packages/vdom-extension/src/index.ts Show resolved Hide resolved
notebooks: INotebookTracker,
restorer: ILayoutRestorer
) => {
const tracker = new InstanceTracker<MimeDocument>({
Copy link
Member

@ian-r-rose ian-r-rose Apr 1, 2019

Usually the base package defines the tracker token, and then the extension provides it. This is so that other extensions can import the token without including the *-extension package.

notebooks: INotebookTracker,
restorer: ILayoutRestorer
) => {
const tracker = new InstanceTracker<MimeDocument>({
Copy link
Member

@ian-r-rose ian-r-rose Apr 1, 2019

It's not required, however. If you want to do it here, that's cool, but I think it can also be done in a follow-up.

@gnestor
Copy link
Contributor Author

@gnestor gnestor commented Apr 2, 2019

This is ready!

Copy link
Member

@ian-r-rose ian-r-rose left a comment

Looks great!

@ian-r-rose ian-r-rose merged commit 6b3c87c into jupyterlab:master Apr 5, 2019
9 checks passed
@gnestor
Copy link
Contributor Author

@gnestor gnestor commented Apr 5, 2019

Hallelujah!! 🎉

@gnestor gnestor deleted the vdom-events branch Apr 5, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Aug 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants