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

move code to example: composition vs inheritance #913

Closed
wants to merge 1 commit into from

Conversation

cyan33
Copy link
Contributor

@cyan33 cyan33 commented May 26, 2018

As is mentioned in #246, I'm trying to move the code from docs/composition-vs-inheritance to /examples. Just want to send a small PR to see if I'm doing it the right way so that I could move on for the rest work.

There are several things that I noticed though:

  1. I tried to convert codepen link to a codesandbox one, but the thing is: there is no global variable name React and ReactDOM in codesandbox. So I had to add it manually through import but it clearly adds noise to the previous compact example. Is there a quick fix about this? (I'm not quite familiar with codesandbox)
  2. What if the codepen example contains css styles? How can I combine all of those to a js file? Or should I come up a new code example maybe?

cc @washingtonsoares @bvaughn

@reactjs-bot
Copy link

reactjs-bot commented May 26, 2018

Deploy preview for reactjs failed.

Built with commit 5f552b5

https://app.netlify.com/sites/reactjs/deploys/5b0f84e611b73b170df22149

);
}
```
`embed:composition-vs-inheritance/split-pane-example.js`

[Try it on CodePen.](https://codepen.io/gaearon/pen/gwZOJp?editors=0010)

Choose a reason for hiding this comment

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

Hello!

It's the first time I'm reading the code and I'm wondering.
Why don't we move this example to CodeSandbox?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah my intention was to move it to CodeSandbox. But in this codepen example, we have both css and javascript and I'm not sure about how to do that within a single javascript file in the example folder. Any thoughts?

Choose a reason for hiding this comment

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

One question: Is it possible to reference multiple files through a codesandbox: // link? If it is possible, I see that it is possible to solve the problem if we create a style.css file and reference it in index.js.

Choose a reason for hiding this comment

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

Another idea would be to create css-in-js, but I'm afraid to cause some unnecessary 'complexity' in the documentation.

What you think about ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it possible to reference multiple files through a codesandbox: // link?

I've tried that but it didn't work. Is it a gatsby plugin that we are using? Maybe should see how that works.

Another idea would be to create css-in-js...

For the second approach, I think it could add much extraneous noise to the example snippets, which is meant to be clear and compact.

@bvaughn
Copy link
Contributor

bvaughn commented May 29, 2018

I tried to convert codepen link to a codesandbox one, but the thing is: there is no global variable name React and ReactDOM in codesandbox. So I had to add it manually through import but it clearly adds noise to the previous compact example. Is there a quick fix about this? (I'm not quite familiar with codesandbox)

The short answer is, " add the import".

We could do something kind of hacky to expose the import as a global var, but it might confuse users who fork the sandbox.

What if the codepen example contains css styles? How can I combine all of those to a js file? Or should I come up a new code example maybe?

Might be better to pick a different example for now.

CodeSandbox API supports adding multiple files- so we could also send a CSS file. But the Gatsby plug-in I wrote doesn't support that yet. Maybe you'd be interested in adding that ability to the plugin as an initial step?

@cyan33
Copy link
Contributor Author

cyan33 commented May 29, 2018

@bvaughn That sounds interesting. Could you show me the link to the gatsby plugin you wrote?

@washingtonsoares
Copy link

washingtonsoares commented May 29, 2018

Maybe you'd be interested in adding that ability to the plugin as an initial step?

I think it's good to add the plugin's ability to support multiple files.

@washingtonsoares
Copy link

Could you show me the link to the gatsby plugin you wrote?

@cyan33 I think this is the link: https://github.com/gatsbyjs/gatsby/tree/master/packages/gatsby-remark-code-repls

@bvaughn
Copy link
Contributor

bvaughn commented May 29, 2018

I'm thinking you could pass it a comma-separated list of files. This would be a backwards compatible change too.

So a single file would continue to be:

[codepen://components-and-props/rendering-a-component.js]

And multiple files would become:

[codepen://components-and-props/rendering-a-component.js,components-and-props/rendering-a-component.css]

@cyan33
Copy link
Contributor Author

cyan33 commented May 29, 2018

That seems good. Working on it now.

@washingtonsoares
Copy link

Hello guys!

All the pull requests that refer to this pull request have already been merged.
I would be very happy if we could finalize this too! 😃

I can help by resolving conflicts and reviewing the code.

What do you think ? @cyan33 @bvaughn

deblasis added a commit to reactjs/it.react.dev that referenced this pull request Feb 26, 2019
used CodeSandox as it's happening upstream:
reactjs/react.dev#913

...which allows uploading CSS (currently we can't with CodePen)
LucaBlackDragon pushed a commit to reactjs/it.react.dev that referenced this pull request Feb 28, 2019
* composition-vs-inheritance: done

used CodeSandox as it's happening upstream:
reactjs/react.dev#913

...which allows uploading CSS (currently we can't with CodePen)

* Prettier fix/offsets missed on 1 file

* Update content/docs/composition-vs-inheritance.md

Co-Authored-By: deblasis <[email protected]>
@washingtonsoares
Copy link

@bvaughn @cyan33 Do you think it would be a good idea to continue this work? I think it will not be so difficult, since the PRs related in Gatsby have already been merged.

@cyan33
Copy link
Contributor Author

cyan33 commented Mar 14, 2019

@washingtonsoares Hey Washington, hope you've doing well and thanks for following up on this pull request. I'll take a look after work!

@BartoszKlonowski
Copy link
Collaborator

Hello @cyan33!
I was walking through that part of docs and saw that the composition-vs-inheritance does no longer exists in the docs and I was unable to find any exact replacement. We already have a bunch of good examples in Passing Props to a Component page, so I don't think this PR you created is still required.
Let me close this as outdated, but thank you very much for taking the time to contribute to the docs! 👍

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

Successfully merging this pull request may close these issues.

6 participants