-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
auto/manual init w/ single bundle, BYO root element 🎉 #1173
Conversation
Deploy preview for netlify-cms-www ready! Built with commit 4caba01 |
Deploy preview for cms-demo ready! Built with commit 4caba01 |
64c35a7
to
60a1089
Compare
eca9877
to
c40636f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to fix this line.
Testing now 👍
I like these changes better than mine 😄
src/bootstrap.js
Outdated
if (document.getElementById(ROOT_ID)) { | ||
console.error('Bootstrap attempted, but Netlify CMS is already initialized!'); | ||
return; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe this is a good practice, because we should allow for the existance of <div id="nc-root" />
, so we can mount the cms to a rendered element in another app.
Suggestion: We check for the bootstrap trying to render onto an already rendered CMS instead. (ie an error calling init() twice in a row without the unmount of the CMS.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you were looking at an old commit, this was removed.
src/bootstrap.js
Outdated
*/ | ||
const newRoot = document.createElement('div'); | ||
newRoot.id = ROOT_ID; | ||
document.body.appendChild(el); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs to be
document.body.appendChild(newRoot);
@talves after you came up with the idea of using a DOM element to trigger manual init, I took a second look at the whole thing and added a commit to really simplify everything by separating manual init and using a custom mount element. We can just use a global flag for manual init so that a custom element is optional. Using a single bundle removes the risk of dual init, so we don't need those warnings anymore. Removing that apparatus also made resetting as simple as rerunning the init function, so it's pretty much gloves off if you use manual init, no safety rails :) Let me know what you think. |
@erquhart I really like the simplification, but it introduced a side affect in the create-react-app test. I will take another look to see why and try to get back to you soon |
Testing now, looks like there are a few errors. |
Fixed. |
Ok, all tested and working as expected. 👍 |
* ensure that application is only initialized once * allow for single bundle init * enable manual initialization via global flag
No description provided.