Make the DOM ready before executing the Script

[anisur2805]

Issue no 61233

What?

The script is running before the DOM is loaded in the Gutenberg docs guideline.

Why?

When I try to follow the guidelines, I get this error: load-scripts.php?c=1…ct-dom&ver=6.5.3:31 Uncaught Error: Minified React error 299

Testing Instructions

Just follow the instructions in this URL.

Testing Instructions for Keyboard

I tested this documentation reference on both Windows and Linux and encountered the same issue. The documentation instructions need to be updated.

Screenshots or Screencast

[github-actions]

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: anisur2805 <anisur8294@git.wordpress.org>
Co-authored-by: stokesman <presstoke@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[github-actions]

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @anisur2805! In case you missed it, we'd love to have you join us in our Slack community.

If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information.

[stokesman]

Hi @anisur2805, thanks for the PR! I was able to reproduce the issue by following the instructions in the docs so it’d be great to get them fixed. The proposed change works yet I'd rather do a little differently as the example code could have been simpler to begin with. If the script is registered with the $in_footer argument true then it won’t need to wait on the document or window events. Here’s diff to demonstrate my suggestion:

diff --git a/docs/how-to-guides/data-basics/1-data-basics-setup.md b/docs/how-to-guides/data-basics/1-data-basics-setup.md
index e61db83c4e..ddfe218052 100644
--- a/docs/how-to-guides/data-basics/1-data-basics-setup.md
+++ b/docs/how-to-guides/data-basics/1-data-basics-setup.md
@@ -25,15 +25,7 @@ function MyFirstApp() {
 }
 
 const root = createRoot( document.getElementById( 'my-first-gutenberg-app' ) );
-window.addEventListener(
-	'load',
-	function () {
-		root.render(
-			<MyFirstApp />,
-		);
-	},
-	false
-);
+root.render( <MyFirstApp /> );
 ```
 
 **style.css:**
@@ -142,7 +134,8 @@ function load_custom_wp_admin_scripts( $hook ) {
 		'my-first-gutenberg-app',
 		plugins_url( 'build/index.js', __FILE__ ),
 		$asset_file['dependencies'],
-		$asset_file['version']
+		$asset_file['version'],
+		true
 	);
 	wp_enqueue_script( 'my-first-gutenberg-app' );
 

What do you think?

Besides this file, in the next part of the tutorial there is place where the index.js is displayed again and it should also be updated to match.

Additionally if you’re up for it, at the end of the "Building an edit form" page/part it has an outdated "(coming soon)" instead of link to the next part. If you want to add the link in this PR, I think that’d be good.

[anisur2805]

Hello @stokesman, as per your recommendation, I have revised the JavaScript script for the next page and updated the URL for 'Building a New Page.'

Thank you.

[stokesman]

Thank you for updating this @anisur2805 and getting that link added. Overall this is an improvement because it works. I still think that listening for two different events is a convoluted way to do this and would prefer the suggestion I made earlier. The only reason I've thought of for not doing that way is it wouldn’t be identical to how the example in the linked repo does it but I figured that could be updated as well. Here’s how it’s being done there:

window.addEventListener(
	'load',
	function () {
		const rootDomElement = document.getElementById(
			'my-first-gutenberg-app'
		);
		const root = createRoot( rootDomElement );
		root.render( <MyFirstApp /> );
	},
	false
);

It’s simpler because it only involves adding one listener. I’d be okay with merging this as is because there’s no good reason to keep the docs broken. However, I'd appreciate it if you’re willing to update this again to use that.

---

I happened to notice one other issue but it’s something that could be treated as a separate issue. createRoot is imported from 'react-dom' and that creates a warning in the console when actually testing the app. The example repo imports it from @wordpress/element instead and that’s probably the easiest fix. I'm uncertain if it’s "right" because it may be preferred to start updating docs to import from react directly.

[anisur2805]

Hi @stokesman, updated the code by removing the extra event listener.

Yes, you are right, the React import issue will be handled separately.

Thank you.

[stokesman]

Looks good! I made sure to test with a plugin built from these steps. I noticed the link to the next part I asked to be added wasn’t using a relative link and its text didn’t match the title of the page it goes to so I pushed d6c0faa to update that.

Thank you for tackling this @anisur2805. I say it’s ready to go.

[stokesman]

Hi @anisur2805, I see the latest change you added was in an unrelated part of the docs. I see the issue and agree it’d be good to fix but in the interest of keeping this PR focused I’d rather revert it here and make a separate PR for it.