Skip to content
This repository has been archived by the owner on Nov 6, 2019. It is now read-only.

Add support for React-like custom element functions #281

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

gnestor
Copy link

@gnestor gnestor commented Jun 7, 2017

This allows phosphor to render custom elements in JSX defined as:

const CustomElement = props => <button onClick={props.onClick}>{props.label}</button>

This allows JSX to render custom elements defined as `const
CustomElement = props = > <button
onclick={props.onclick}>{props.label}</button>`
@sccolbert
Copy link
Member

Thanks for this!

Because of the camel-case changes, this is a breaking API change. If I release a 2.0 of this, it will force widgets to go to 2.0 as well. I'd rather not do that right now, so I'd like to thing of a way we can support both. I have an idea of how to this, just buried under grid work right now.

@gnestor
Copy link
Author

gnestor commented Jun 7, 2017

Ok, let's move those commits into a separate PR. I think the element event/attribute names will require some more work to get 100% parity with React.

@sccolbert
Copy link
Member

I wonder if we should put the JSX definitions into a separate .d.ts file. If they are made global like this, I don't think anyone will be able to use the react typings when using phosphor.

@gnestor
Copy link
Author

gnestor commented Jun 7, 2017

I am new to Typescript so that sounds like a good plan 👍

@sccolbert
Copy link
Member

Possibly related: microsoft/TypeScript#8757

I wonder if we can figure out how to define the namespace in Phosphor, then import and use it in another module without needing to make it global for the whole app.

@ellisonbg
Copy link
Contributor

As far as the JSX namespace thing. As long as it is global I worry folks can't use react at the same time. Does putting it into a separate d.ts file help that? The problem is that the typescript compiler is currently expecting JSX to be global, so putting it as a module level namespace doesn't work.

@sccolbert
Copy link
Member

@ellisonbg The typescript issue I linked seemed to be asking for that, and sounds like from the responses it's possible. I may be misunderstanding the conversation though.

@jdetle
Copy link
Contributor

jdetle commented Sep 30, 2017

How is this looking wrt possibly merging it?

@ellisonbg
Copy link
Contributor

I don't think we should merge this PR as we are wanting to maintain less VDOM stuff in phosphor, not more. Also, the JSX typings do conflict with @types/react. While we haven't made a final decision, it is my preference for us to simply user react and deprecate phosphors vdom.

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

Successfully merging this pull request may close these issues.

4 participants