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 vdom-extension #37

Merged
merged 7 commits into from
Oct 6, 2017
Merged

Add vdom-extension #37

merged 7 commits into from
Oct 6, 2017

Conversation

gnestor
Copy link
Collaborator

@gnestor gnestor commented Sep 28, 2017

To do:

  • Use @nteract/transform-vdom

Closes #34

image

@blink1073
Copy link
Contributor

I think this should have a README that mentions https://github.com/nteract/vdom and https://github.com/nteract/vdom/blob/master/docs/spec.md. Otherwise looks great!

@@ -0,0 +1,83 @@
import * as React from 'react';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

License?

Copy link
Member

@rgbkrk rgbkrk Sep 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Originally MIT, I heavily modified REON to make this non-mutable. Side note -- can you all use packages from nteract if we hold ourselves to maintaining semver appropriately? This means there are now three copies of this floating around -- the one in nteract/nteract/packages/transform-vdom, the current jupyter notebook PR, and this PR

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Depending on it here sounds good, not much we can do about notebook (unless you want to support bower).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy to produce AMD since I need to keep supporting classic at work.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As in React license issue? React 15.6.2 is licensed under MIT so we should be fine to use that. json-extension also depends on React.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I meant the license of this file in particular, since I saw that Kyle had included one inline in the notebook PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a side note, I do find it odd that code is bulk lifted from nteract. We do publish these packages directly, and they get depended on by nteract desktop, hydrogen, and commuter. @nteract/transform-vdom is available on npm and takes the raw data (standard JS, no immutable) in as a prop data={myJSON}. We can easily publish the object-to-react code too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rgbkrk Ya, I totally agree that the frontends should all consume the same renderer package. I tried to add @nteract/transform-vdom as a dependency but I'm having trouble providing Typescript definitions for it 👎

Copy link

@ellisonbg ellisonbg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Left a few comments inline, I will leave it up to you about how to address them (now versus later).

/**
* The CSS class to add to the VDOM Widget.
*/
const CSS_CLASS = 'jp-RenderedVDOM';

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are actually starting to move away from declarring CSS classes as module level const variables. Not a big deal, but in the future, we can just put them inline in the code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you can point me to a good extension/plugin reference, I can update all of these extensions to conform to that style.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is the pattern used in Phosphor currently, but we haven't yet updated JupyterLab with the new pattern.

@@ -0,0 +1,35 @@
<?xml version="1.0" encoding="utf-8"?>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@ellisonbg
Copy link

ellisonbg commented Sep 28, 2017 via email

@rgbkrk
Copy link
Member

rgbkrk commented Sep 28, 2017

where are you example notebooks for this? Maybe make the vdom python package a binder with notebooks ;-)

wellllll I need jupyter/notebook#2840 merged and shipped before that will be possible

There's an open issue for example notebooks in nteract/vdom#4, though I now have quite a few demo notebooks locally now. I was thinking of making a youtube tutorial / demo later today (barring progress on jupyter/notebook#2871).

@rgbkrk
Copy link
Member

rgbkrk commented Sep 28, 2017

I think the coolest little revelation about the vdom type is that you can write pure python functions to compose together new layouts, given regular python data. This alleviates my desire for a top level container type (like I brought up elsewhere):

screen shot 2017-09-28 at 12 43 33 pm

@blink1073
Copy link
Contributor

Sounds like vdom could use a helper function to convert a matplotlib figure to a source string so you can write img(src=srcFromFig(fig)).

@rgbkrk
Copy link
Member

rgbkrk commented Sep 28, 2017

😅 Yeah I definitely did some hackery for that... I wish matplotlib was a bit easier to read a raw image from (I'd prefer to post these to an image service anyways, not encoding it as base64 encoded images).

I have this maintenance feeling that vdom should be just low level building blocks and that we should provide helpers in some library built on top of it. Mayyyybe it's ok to put it in a module that exists within. ¯\_(ツ)_/¯ I am, of course, totally open to PRs exploring this. 😄

@ellisonbg
Copy link

ellisonbg commented Sep 29, 2017 via email

@gnestor
Copy link
Collaborator Author

gnestor commented Sep 29, 2017

@ellisonbg Does it make sense to put example notebooks in this repo? I could then create a binder postScript to install the extensions and then people could open the example notebooks to see the renderers in action!

@gnestor gnestor force-pushed the vdom branch 6 times, most recently from 79c4a1d to b84e171 Compare October 3, 2017 17:45
@gnestor
Copy link
Collaborator Author

gnestor commented Oct 3, 2017

Moved package README commits to #39 and binder commits to #40

@gnestor gnestor force-pushed the vdom branch 2 times, most recently from 0f26e05 to 4e6d693 Compare October 3, 2017 18:05
@blink1073
Copy link
Contributor

@gnestor, I'd say make a local typings file for the one function we use in @nteract/transform-vdom similar to https://github.com/jupyterlab/jupyterlab/blob/master/typings/json-to-html/json-to-html.d.ts.

@gnestor
Copy link
Collaborator Author

gnestor commented Oct 6, 2017

Here is my working branch with local type definitions for @nteract/trasnform-vdom: https://github.com/gnestor/jupyter-renderers/tree/transform-vdom

Diff: gnestor@cf9c699

I'm getting the following TS error:

src/index.tsx(16,23): error TS7016: Could not find a declaration file for module '@nteract/transform-vdom'. '/Users/grant/Sites/jupyter/jupyter-renderers/node_modules/@nteract/transform-vdom/lib/index.js' implicitly has an 'any' type.
src/transform-vdom.d.ts(21,16): error TS2665: Invalid module name in augmentation. Module '@nteract/transform-vdom' resolves to an untyped module at '/Users/grant/Sites/jupyter/jupyter-renderers/node_modules/@nteract/transform-vdom/lib/index.js', which cannot be augmented.

Would appreciate some expert TS advice 👍

@rgbkrk
Copy link
Member

rgbkrk commented Oct 6, 2017

We'd happily maintain and ship the TS definitions if that helps, I'd just need a starter version in the repo.

@blink1073
Copy link
Contributor

blink1073 commented Oct 6, 2017

If we add a types: <path> to the package.json in @nteract/trasnform-vdom and put this in that file it works:

import * as React from 'React';

interface VDOMElement {
  tagName: 'string';
  attributes: Object;
  children: Array<VDOMElement>;
  key?: number | string | null;
}

interface VDOMProps extends React.Props<VDOM> {
  data: VDOMElement;
}

declare class VDOM extends React.Component<VDOMProps, {}> { }

export default VDOM;

@gnestor
Copy link
Collaborator Author

gnestor commented Oct 6, 2017

@rgbkrk I think that would be a good idea moving forward. I just want to merge this and move it over the to jupyterlab repo so people can start using it!

@rgbkrk
Copy link
Member

rgbkrk commented Oct 6, 2017

Ok cool I'll get that in today. I fixed some other edge cases too.

Fix type definitions for `@nteract/transform-vdom`
@gnestor
Copy link
Collaborator Author

gnestor commented Oct 6, 2017

Ok, got the local typing working! 🙌 I think this is ready to merge!

@blink1073
Copy link
Contributor

Nice! Merging.

@blink1073 blink1073 merged commit a7f9609 into jupyterlab:master Oct 6, 2017
@gnestor gnestor deleted the vdom branch October 6, 2017 18:13
@gnestor
Copy link
Collaborator Author

gnestor commented Oct 6, 2017

Ok, VDOM mime type working in lab!: https://beta.mybinder.org/v2/gh/jupyterlab/jupyter-renderers/master (just go to the /lab vs. /tree endpoint)

Next, we gotta get jupyter/notebook#2840 passing so that we have support in the notebook.

@rgbkrk
Copy link
Member

rgbkrk commented Oct 6, 2017

wooooo

@ellisonbg
Copy link

ellisonbg commented Oct 6, 2017 via email

p(['Can you believe we wrote this ', b('in Python'), '?']),
img(src="https://media.giphy.com/media/xUPGcguWZHRC2HyBRS/giphy.gif"),
p(['What will ', b('you'), ' create next?']),
])
Copy link
Member

@rgbkrk rgbkrk Oct 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realize this is merged now -- if you want this to be a little bit more trim vdom allows positional arguments instead of arrays, so you can now write:

from IPython.display import display
from vdom.helpers import h1, p, img, div, b

display(
    div(
        h1('Our Incredibly Declarative Example'),
        p('Can you believe we wrote this ', b('in Python'), '?'),
        img(src="https://media.giphy.com/media/xUPGcguWZHRC2HyBRS/giphy.gif"),
        p('What will ', b('you'), ' create next?'),
    )
)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#44

<circle fill="#61DAFB" cx="420.9" cy="296.5" r="45.7"/>
<polygon fill="#61DAFB" points="520.5,78.1 520.5,78.1 520.5,78.1 "/>
</g>
</svg>
Copy link
Member

@rgbkrk rgbkrk Oct 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from vdom import createComponent
svg = createComponent('svg')
g = createComponent('g')
path = createComponent('path')
polygon = createComponent('polygon')
circle = createComponent('circle')

react_color = "#61DAFB"
jupyter_color = "#EE611E"

color = jupyter_color

orbital_paths = """M666.3,296.5c0-32.5-40.7-63.3-103.1-82.4c14.4-63.6,8-114.2-20.2-130.4c-6.5-3.8-14.1-5.6-22.4-5.6v22.3
		c4.6,0,8.3,0.9,11.4,2.6c13.6,7.8,19.5,37.5,14.9,75.7c-1.1,9.4-2.9,19.3-5.1,29.4c-19.6-4.8-41-8.5-63.5-10.9
		c-13.5-18.5-27.5-35.3-41.6-50c32.6-30.3,63.2-46.9,84-46.9l0-22.3c0,0,0,0,0,0c-27.5,0-63.5,19.6-99.9,53.6
		c-36.4-33.8-72.4-53.2-99.9-53.2v22.3c20.7,0,51.4,16.5,84,46.6c-14,14.7-28,31.4-41.3,49.9c-22.6,2.4-44,6.1-63.6,11
		c-2.3-10-4-19.7-5.2-29c-4.7-38.2,1.1-67.9,14.6-75.8c3-1.8,6.9-2.6,11.5-2.6l0-22.3c0,0,0,0,0,0c-8.4,0-16,1.8-22.6,5.6
		c-28.1,16.2-34.4,66.7-19.9,130.1c-62.2,19.2-102.7,49.9-102.7,82.3c0,32.5,40.7,63.3,103.1,82.4c-14.4,63.6-8,114.2,20.2,130.4
		c6.5,3.8,14.1,5.6,22.5,5.6c27.5,0,63.5-19.6,99.9-53.6c36.4,33.8,72.4,53.2,99.9,53.2c8.4,0,16-1.8,22.6-5.6
		c28.1-16.2,34.4-66.7,19.9-130.1C625.8,359.7,666.3,328.9,666.3,296.5z M536.1,229.8c-3.7,12.9-8.3,26.2-13.5,39.5
		c-4.1-8-8.4-16-13.1-24c-4.6-8-9.5-15.8-14.4-23.4C509.3,224,523,226.6,536.1,229.8z M490.3,336.3c-7.8,13.5-15.8,26.3-24.1,38.2
		c-14.9,1.3-30,2-45.2,2c-15.1,0-30.2-0.7-45-1.9c-8.3-11.9-16.4-24.6-24.2-38c-7.6-13.1-14.5-26.4-20.8-39.8
		c6.2-13.4,13.2-26.8,20.7-39.9c7.8-13.5,15.8-26.3,24.1-38.2c14.9-1.3,30-2,45.2-2c15.1,0,30.2,0.7,45,1.9
		c8.3,11.9,16.4,24.6,24.2,38c7.6,13.1,14.5,26.4,20.8,39.8C504.7,309.8,497.8,323.2,490.3,336.3z M522.6,323.3
		c5.4,13.4,10,26.8,13.8,39.8c-13.1,3.2-26.9,5.9-41.2,8c4.9-7.7,9.8-15.6,14.4-23.7C514.2,339.4,518.5,331.3,522.6,323.3z
		 M421.2,430c-9.3-9.6-18.6-20.3-27.8-32c9,0.4,18.2,0.7,27.5,0.7c9.4,0,18.7-0.2,27.8-0.7C439.7,409.7,430.4,420.4,421.2,430z
		 M346.8,371.1c-14.2-2.1-27.9-4.7-41-7.9c3.7-12.9,8.3-26.2,13.5-39.5c4.1,8,8.4,16,13.1,24C337.1,355.7,341.9,363.5,346.8,371.1z
		 M420.7,163c9.3,9.6,18.6,20.3,27.8,32c-9-0.4-18.2-0.7-27.5-0.7c-9.4,0-18.7,0.2-27.8,0.7C402.2,183.3,411.5,172.6,420.7,163z
		 M346.7,221.9c-4.9,7.7-9.8,15.6-14.4,23.7c-4.6,8-8.9,16-13,24c-5.4-13.4-10-26.8-13.8-39.8C318.6,226.7,332.4,224,346.7,221.9z
		 M256.2,347.1c-35.4-15.1-58.3-34.9-58.3-50.6c0-15.7,22.9-35.6,58.3-50.6c8.6-3.7,18-7,27.7-10.1c5.7,19.6,13.2,40,22.5,60.9
		c-9.2,20.8-16.6,41.1-22.2,60.6C274.3,354.2,264.9,350.8,256.2,347.1z M310,490c-13.6-7.8-19.5-37.5-14.9-75.7
		c1.1-9.4,2.9-19.3,5.1-29.4c19.6,4.8,41,8.5,63.5,10.9c13.5,18.5,27.5,35.3,41.6,50c-32.6,30.3-63.2,46.9-84,46.9
		C316.8,492.6,313,491.7,310,490z M547.2,413.8c4.7,38.2-1.1,67.9-14.6,75.8c-3,1.8-6.9,2.6-11.5,2.6c-20.7,0-51.4-16.5-84-46.6
		c14-14.7,28-31.4,41.3-49.9c22.6-2.4,44-6.1,63.6-11C544.3,394.8,546.1,404.5,547.2,413.8z M585.7,347.1c-8.6,3.7-18,7-27.7,10.1
		c-5.7-19.6-13.2-40-22.5-60.9c9.2-20.8,16.6-41.1,22.2-60.6c9.9,3.1,19.3,6.5,28.1,10.2c35.4,15.1,58.3,34.9,58.3,50.6
		C644,312.2,621.1,332.1,585.7,347.1z"""

svg(
    g(
        path(fill="currentColor", d=orbital_paths),
        polygon(fill="currentColor", points="320.8,78.4 320.8,78.4 320.8,78.4"),
        circle(fill="currentColor", cx="420.9", cy="296.5", r="45.7"),
        polygon(fill="currentColor", points="520.5,78.1 520.5,78.1 520.5,78.1")
    ), version="1.1", id="Layer_2_1_", x="0px", y="0px", viewBox="0 0 841.9 595.3", enableBackground="new 0 0 841.9 595.3", height="128", width="128", color=jupyter_color
)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

screen shot 2017-10-06 at 1 32 50 pm

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

@rgbkrk rgbkrk Oct 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That show is so good, have you been watching it?

Edit: and crass, and terrible, and a good approximation of what it was like to be a teenager going through puberty

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just saw that it was featured on Netflix. Haven't seen it yet...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heh, it's only extra funny because my wife and I laughed really hard at that scene.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Watched the first episode last nite 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants