Add ScreenShot Preview Block #220
Conversation
|
Rather than create an entire new project (config files, package.json, etc), could you try out the build process & template I proposed in #211 & WordPress/wporg-repo-tools#21? |
|
Yep makes sense. This block was created in a galaxy far far away...I didn't know it would end up here :) |
|
I would like to get this out as a beta on .org, so I'll merge this now and follow up later. If there are any issues, I'll follow them up asap. |
| @@ -0,0 +1,84 @@ | |||
| <?php | |||
| /** | |||
| * Plugin name: Gutenberg: Pattern Previewer | |||
There was a problem hiding this comment.
There's a bit of inconsistency between the naming and description here and in block.json. This file is quite particular about it being for previewing patterns, whereas block.json seems more generic. Is that intentional?
There was a problem hiding this comment.
Nope, just missed this. Nice catch.
| * License: GPLv2 or later | ||
| */ | ||
|
|
||
| namespace WordPressdotorg\Gutenberg\ScreenshotPreview; |
There was a problem hiding this comment.
Gutenberg -> MU_Plugins ?
| window.addEventListener( 'resize', handleOnResize ); | ||
|
|
||
| return () => { | ||
| window.addEventListener( 'resize', handleOnResize ); |
There was a problem hiding this comment.
If this is intended to clean up should it be using removeEventListener?;
There was a problem hiding this comment.
Great catch!
| style={ { | ||
| height: frameHeight, | ||
| } } | ||
| tabIndex="-1" |
There was a problem hiding this comment.
Is this to stop keyboard navigation? If so why?
| "license": "ISC", | ||
| "devDependencies": { | ||
| "@wordpress/scripts": "^23.4.0", | ||
| "lodash": "^4.17.21" |
There was a problem hiding this comment.
Looks like this should be in dependencies
| /** | ||
| * WordPress dependencies | ||
| */ | ||
| import { useEffect, useState } from '@wordpress/element'; |
There was a problem hiding this comment.
I don't see this package in package.json dependencies
| return; | ||
| } | ||
|
|
||
| for ( let i = 0; i < blockElements.length; i++ ) { |
There was a problem hiding this comment.
Any reason not to use forEach?
There was a problem hiding this comment.
What are the arguments for using forEach?
There was a problem hiding this comment.
This pretty much sums it up https://stackoverflow.com/questions/43031988/javascript-efficiency-for-vs-foreach#answer-43032526
Personally as a dev I much prefer to write and read less code. I don't think the perf benefit with a for loop outweighs this in most cases. I tend to stick to all the modern native array functions: forEach, map, some, reduce
| import { __ } from '@wordpress/i18n'; | ||
| import { useEffect, useRef, useState } from '@wordpress/element'; | ||
| import { Spinner } from '@wordpress/components'; |
There was a problem hiding this comment.
Missing from package.json dependencies?
adamwoodnz
left a comment
adamwoodnz
left a comment
There was a problem hiding this comment.
Nice work, this is a cool block!
A few minor comments/questions inline, I may be missing some context/know-how though, if so sorry!
This block was created to add pattern previews to the theme directory in this Pull Request.
Block Summary:
mShotsservice.