Comment to help user understand 'mountNode' on home page example. #1019
Conversation
This is to help users understand the mountNode variable on the home page of reactjs.org sicne currently it offers no explanation. This lack of an explanation might confuse users. eg: https://stackoverflow.com/questions/23600443/uncaught-referenceerror-mountnode-is-not-defined
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed. If you have received this in error or have any questions, please contact us at [email protected]. Thanks! |
craigdanj
changed the title
|
Deploy preview for reactjs ready! Built with commit 5b4daa3 |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
|
This is a good change, but the formatting in the front page code blocks is very narrow. Can you look at the deploy preview and figure out how to wrap the comment so it fits? Thanks! |
There was a problem hiding this comment.
To be honest I'm not sure this is very helpful.
Formatting isn't the only issue. We only have so much we can tell in these examples. If this single line is worth a lengthy explanation, why aren't other lines also worth it? Why are we over-explaining this particular line in this particular example?
I don't think this solution is good. Maybe instead we can rename mountNode to something more meaningful. Or maybe it should be something like
document.getElementById('first-example')which is self-explanatory.
|
Maybe we could pull the react-dom part into the home page examples plugin? Mountnode is defined there and needed to make the examples work with live edits. |
|
@gaearon I actually like your idea better than mine. I agree mine isn't the best solution. Can we replace 'mountNode' with document.getElementById('root') In all the examples? Would that make sense? |
|
I think it will be confusing to have |
This is as per @Gearon 's suggestion in the comments.
|
@gaearon I've updated the first example to use I can change the other examples if this is fine. |
…as render element.
…l-plugin-example'
|
@gaearon So I've updated all the examples to use getElementById() and have used ids that correspond to each example's title (like you suggested) since this makes more sense for the documentation. Do let me know if the naming is appropriate and if there is anything else I need to change since the main website has live code editing. |
This is still a bit opaque but it's difficult to fix with the current way CodeEditor works: - long-term CodeEditor.js could take a node reference instead of defining mountNode internally - could also use document.createElement to define the target in the code, but this could be mislead people to think this is required instead of using an existing reference see #1017, #1018, #1019
|
I did something similar in #1526. I couldn't merge this PR because it didn't actually work, but thanks for starting it! |
#1017
This is to help users understand the mountNode variable on the home page of reactjs.org sicne currently it offers no explanation. This lack of an explanation might confuse users.
eg: https://stackoverflow.com/questions/23600443/uncaught-referenceerror-mountnode-is-not-defined