#5303 closed task (blessed) (fixed)
Block plugin validation tool UI improvements
Reported by: | tellyworth | Owned by: | |
---|---|---|---|
Milestone: | Priority: | normal | |
Component: | Plugin Directory | Keywords: | |
Cc: |
Description
The UI for the forthcoming block validator tool is very rough and needs improvement.
Attachments (18)
Change History (113)
@
4 years ago
Allow passing in github branches, ex: https://github.com/ryelle/rmb-recipe-block/tree/try/block-json
#5
follow-up:
↓ 7
@
4 years ago
single-parent-block.diff This check is just an idea, I don't think we've officially made it a rule — there should be only one "top level" block per block-plugin. Having a single block ID'd as the "main block" would be helpful for the block dir API too, make sure we're showing the right block name.
Currently this check only works with block.json files, since we can't/don't get the rest of the config settings from the JS or PHP calls. So for now, it's just a warning, and it could be incorrect.
#6
@
4 years ago
style-plugin.diff & style-theme.diff are a pair, some markup & CSS fixes to clean up the page a bit. I'm not sure I made the change in the right place in the theme, so I can move that around if needed.
#7
in reply to:
↑ 5
@
4 years ago
Replying to ryelle:
single-parent-block.diff This check is just an idea, I don't think we've officially made it a rule — there should be only one "top level" block per block-plugin. Having a single block ID'd as the "main block" would be helpful for the block dir API too, make sure we're showing the right block name.
Yep this is great. At the moment there's a much lazier check which blindly assumes that < 5 blocks might be parent/child blocks but 5+ is too many. It definitely should be a rule that there's only one top level block. I did have an earlier idea that we could enforce only one block.json
file per plugin, with that file representing the top-level block. But this is much more comprehensive. It seems reasonable to say "if you want to include multiple parent/child blocks, you need to use block.json
files for all of them with the correct parent
values."
Let's put this in, and update the 5 block rule in check_for_blocks
to be a higher number. At some point if there are, I dunno, 15 or 20 blocks in there, even if the parent/child relationship is configured correctly it might be a sign that something's gone wrong.
This ticket was mentioned in Slack in #core-editor by ryelle. View the logs.
4 years ago
#14
@
4 years ago
I think the change in r10040 (checking for one parent) may be too aggro. For example, listicles gets flagged:
More than one top level block was found: Listicles, Listicle Item Title, List Item, List Item Content More about this.
But the reason is there are multiple children (grandchildren)
- Listicles - parent block
- List Item - individual item, child of Listicle
- List title & List Content -- children of List Item, grandchildren of Listicles
Is there a way to account for that? I'm not the only person who uses that methodology :)
#15
@
4 years ago
If you add a block.json for each block, and specify that parent relationship it should understand that — so List Item should have a property parent: [listicle]
; list title & list content should have parent: [list item]
(using the block names).
We can't get that info out of the JS or PHP files, unfortunately.
#16
@
4 years ago
collapsible-details-plugin.diff & collapsible-details-theme.diff switch away from the external help page, and add inline expandable help text to each check type. I think this gives a better dev experience, and will be translatable. The downside is needing a code change to update docs, but it seems that's how the other pages work.
There are still todo notes & other questions in this patch, but it's a first pass at adding descriptions to each check. I also collapsed all the check_block_json_is_valid
issues into one section, since it can get wordy :)
Speaking of check_block_json_is_valid
, I think it's a little aggressive — my test block uses script handles for editorScript, and it's flagging every one of them since they don't end in .js
. Is that a requirement for finding the assets in the API?
I also had the idea to make a check for a well-formed block name— it should be alphanumeric, start with a letter, and contain one slash for namespace. If we agree that this should be the plugin slug, it could also check that. Otherwise a basic check that it isn't one of [ cbg, example, block, core ]
seems good to me (other example prefixes I should add?).
#17
@
4 years ago
If you add a block.json for each block, and specify that parent relationship it should understand that — so List Item should have a property
parent: [listicle]
; list title & list content should haveparent: [list item]
(using the block names).
Yeah that's exactly what I did.
- listitem.js has
parent: [ 'lez-library/listicles' ],
- listdt.js has
parent: [ 'lez-library/listitem' ],
So all it's doing is saying "Hey, you have multiple items picked as parents" which ... yes? Could it not check if the item it picked as parent _also_ has a parent?
If not, it's no big, but we should have a note in the docs that says "grandchildren items may cause a false positive"
#18
@
4 years ago
Hmmmm, it should only be counting blocks that don't have parent
set. And you've got these properties set in the block.json files? What plugin URL are you using to check this?
#19
follow-up:
↓ 21
@
4 years ago
Ah. I have the properties in the JS but NOT the block.json (we really need a better example block.json because right now it's going to drive people bonkers trying to figure anything out -- it needs to be like the readme.txt example)
I was checking https://plugins.svn.wordpress.org/listicles/trunk and NOW with whatever changes just went through it stopped telling me I had too many parents :) (all families are valid, Block checker!)
Which reminds me...
https://wordpress.org/plugins/developers/block-plugin-validator/help/#check_block_tag should clearly explain "Your plugin needs the tag block
" people had been using blocks (plural) because ... who knows.
#20
@
4 years ago
we really need a better example block.json because right now it's going to drive people bonkers trying to figure anything out
I agree, and I think it's finally stable enough that we can do that :) I added an example & more info to these docs, which we can link to from the error messages.
#21
in reply to:
↑ 19
@
4 years ago
Replying to Ipstenu:
Ah. I have the properties in the JS but NOT the block.json (we really need a better example block.json because right now it's going to drive people bonkers trying to figure anything out -- it needs to be like the readme.txt example)
This is a great point. Where does the sample block.json
belong?
people had been using blocks (plural) because ... who knows.
The plural blocks tag makes sense for plugins that contain multiple blocks. I added a check for the singular tag more as a suggestion.
#24
@
4 years ago
I've been testing the Block Plugin Validator the last two days with our Block for Apple Maps plugin and wanted to share the following feedback (note that I'm testing on Firefox 78.0.1.):
NICE TO HAVE
- Widen the text field for "Plugin repo URL" so the entire helper text displays.
- Link to plugin source for the specific revision (in the case of my sample plugin that would be to https://plugins.trac.wordpress.org/browser/maps-block-apple?rev=2338279) as this will allow inspection of the plugin code in comparison to the block validator results.
DESIRED
- I see SUCCESS and a nice green-highlighted message so it looks like I'm all set, except the copy makes it sound like there are additional steps I need to take ("No problems were found. Your plugin has passed the first step towards being included in the Block Directory."). Only its not clear what those steps are. Having details on what happens next is helpful to understand if there's more I need to do or if its now sitting with some other team (PRT perhaps?).
- There are WARNINGS and NOTES sections each with items called out with "More about this." links that in some cases don't hyper/anchor link to anything in particular. If there's help needed on copy for those links I'm happy to help draft those, but having something that is linked to for those would be helpful.
- Its unclear if the WARNINGS are things I need to resolve in order for our block to appear in the Block Directory, but I'd assume things are ok as there are no "ERRORS" which would tell me I still have things to fix. If the WARNINGS need to be resolved, it would be helpful to know.
- On the Block Validator Help page, the "Block scripts" section has copy that stops mid-sentence. Getting that clarified will be helpful.
- In trying to find more details on what is needed for our block plugin to appear in the Block Directory I've searched for and found several different markdown pages on GitHub (Share your Block with the World, Block Type Registration RFC, and Block Registration) each of which provide information that would ideally live in the Plugin Handbook as the canonical source of information where developers go to learn about crafting their plugins (in this case a block plugin). I suspect that for most plugin developers that it is not immediately obvious to find the right markdown file in the right branch in the Gutenberg GitHub repo to figure out what's needed for their block plugin to appear in the Block Directory.
@
4 years ago
Oof, looks like updates happened while I was documenting my previous testing feedback. In the latest revision, I see another issue likely related to my testing in Firefox. Namely that the expand/collapse actions are not obvious and I only stumbled onto them by clicking on a row in the WARNINGS section. There is no visual queue in Firefox that those rows are clickable and that the feedback for each warning row appears once you click on each row.
#33
@
4 years ago
@jeffpaul thanks for running through this! as you've noticed, most of the copy is still in progress. I added a link to the plugin code in r10057. It should always run the checker on trunk, but if you think linking to the exact rev would be helpful I can add that in. The "more about this" links have been removed in favor of the inline comments.
We definitely need better guidance for "what's next" if there are errors & warnings (only errors prevent adding to the directory). Where would you want to see that info?
#34
@
4 years ago
That's odd about the lack of visual cue in FireFox @JeffPaul. I did notice the lack of triangle icons in FF, and thought I'd fixed that with the summary
style addition in r10048. Perhaps I didn't build the CSS correctly when I committed that - can confirm that the triangles are missing again.
#36
@
4 years ago
I forgot to bust the cache, that's why. Triangles should appear now on expandable items in Firefox. It still could use a better visual cue, but that's something at least.
#37
@
4 years ago
I can confirm that Firefox mobile Version 27.0 (18428) correctly shows the triangle icons.
#40
@
4 years ago
Came here to provide feedback about the language and general experience of the block validator. JeffPaul pointed out much of what was on my mind above though! In any case, here's what's on my mind as general enhancements:
- It would be great to have more links to documentation or places to get help. Right now, the experience can feel a bit like a dead end otherwise.
- Put priority on any "warnings" found so they show above any success message. It's odd to me how often there's both a message with warnings yet a "success" message. It's not clear to me if it's actually okay to ignore the warnings (more on this later)
- Make the distinction between separate warnings clearer. Right now, it's a bit confusing that some of the warnings have an arrow to read more and others.
- When reviewing block plugins with fatal errors, warnings, and the "problems were encountered" language it becomes really hard to know what to focus on. I'd recommend changing the visual of "problems were encountered" to make it more distinct from Fatal Errors. Right now, it looks like I need to resolve all three sections to proceed (warnings, problems were encountered, fatal errors) when in actuality I just need to focus in on the fatal errors to start.
- I might consider renaming "Warnings" to "Recommended Improvements" and "Fatal Errors" to "Blockers". This depends on whether warnings are actually blockers though!
This tool will help determine if a WordPress block plugin is suitable for inclusion in the Block Directory. Enter a SVN repository URL, Github URL, or plugin slug to check. Please note this is a work in progress.
To call out a specific example, it would be great to link to some wider context from the start. Perhaps even linking to this recent post for now would be helpful:
#41
@
4 years ago
Here's an attempt at moving stuff around & adding more context: https://cloudup.com/cKNeb_LcIQj (nothing's committed yet, so it's just some result screenshots)
- Moved the overall result to the top of the "Result" section, so it's a little more obvious this is the outcome, and the rest are individual issues
- Tweaked the language on the "success" message since there might be warnings, and switched it to info-style if the plugin is already in the block directory.
- Renamed Warnings to "Recommendations"
- Added descriptions to error & recommendation sections
The pattern of having the individual check results use the error/warning/info notice style could also be updated if that seems overwhelming, thoughts?
Make the distinction between separate warnings clearer.
Do you have any suggestions? I thought about having all these details default to open, but I don't know if that would be too much information.
It would be great to have more links to documentation or places to get help.
I think these efforts are scattered & in-progress right now, but as they come together I agree, we'll want a better intro here.
#42
@
4 years ago
This looks a ton better already! Thanks for digging right in. Much easier to understand what actions need to be taken. Some additional feedback...
- In this screenshot https://cloudup.com/iSeHAh8aemA The language of "Your plugin has passed the first step towards being included in the block directory" is a bit confusing as my brain immediately goes to "What's the next step?". Without the UI having corresponding "step" related direction, it can feel like one's missing something. I might change this to something like "Your plugin has passed the checker tests and is ready to be included in the block directory."
- In this screenshot https://cloudup.com/iXgZw_lNR12 I might change the language from "They need to be addressed before your plugin will work in the Block Directory" to "They need to be addressed in order for your plugin to be included in the Block Directory" to make it clearer that the plugin cannot be added since the checker is meant to be part of the flow for adding to the directory.
- The language around recommendations can be a bit clearer. Perhaps "These are suggestions to improve your block. While they are not required for your block plugin to be added to the Block Directory, addressing them will help people discover and use your block."
Do you have any suggestions? I thought about having all these details default to open, but I don't know if that would be too much information.
I think what might help is that if each warning has more information so there is consistency in every warning having the arrow in front of it. That weirdly makes it easier to distinguish and creates a nice visual pattern. Agreed on default to open being too much.
This ticket was mentioned in Slack in #core-editor by annezazu. View the logs.
4 years ago
#44
@
4 years ago
In this screenshot https://cloudup.com/iSeHAh8aemA The language of "Your plugin has passed the first step towards being included in the block directory" is a bit confusing as my brain immediately goes to "What's the next step?".
The section right under that notice is the next step— the "Add … to Block Directory" button (block authors will see this section as "Committer Tools", without the edit link). How do you think we can make that more clear?
#45
@
4 years ago
I think that button is quite clear. I just think the language around "steps" is going to cause confusion. Usually when there's mention of steps in a flow there's a nice list or a clear continuation of language (ie "Your final step is to.."). Without it, I just think that part might be confusing. This is a bit nitpicky ;) but clarity doesn't hurt.
#47
@
4 years ago
More updates: I removed most of the "next step" language entirely, so that it's more clear that the actual next step is to click the button (and added a message to upload the plugin to wp.org if it's a github scan); added descriptions to all the errors. see screenshots here.
#48
@
4 years ago
@webfactory reported on the make/plugins post:
Block Plugin Checker has a broken link. When a check fails due to “No blocks found in plugin.” it links to https://wordpress.org/plugins/developers/block-plugin-validator/TUTORIAL which redirects to https://wordpress.org/plugins/search/developers/ which is probably not intended to happen
I think the intended link was https://wordpress.org/plugins/developers/block-plugin-validator/help/
If I test the checker for https://plugins.svn.wordpress.org/autoshare-for-twitter, in the "Fatal Errors:" section I see "No blocks found in plugin." and "Learn how to create a block." that links to https://wordpress.org/plugins/developers/block-plugin-validator/TUTORIAL/. I'm not certain that I know of an exact link for a tutorial at this point, but the make/plugins post is the closest I know of for now so maybe we use that link instead?
#49
@
4 years ago
I would say TUTORIAL should link to https://developer.wordpress.org/block-editor/ or https://developer.wordpress.org/block-editor/tutorials/create-block/
#56
@
4 years ago
I've uploaded a patch (5303-translations.diff) that checks for wp_set_script_translations
& warns if it's not found. It's not a block directory requirement, but I've come across a bunch of block directory plugins that have translations but don't add it— so the translations don't work. I'm just not sure if the block checker is the right place for a check like this, or if we should stick to improving documentation.
#57
@
4 years ago
r10101 & r10102 should be attached to this ticket (my mistake in the commits).
r10102 commits the 5303-translations.diff patch.
#61
follow-up:
↓ 62
@
4 years ago
I caught this question in the #core-coding-standards channel and wasn’t sure if this was an odd case we should consider with the checker?
https://wordpress.slack.com/archives/C5VCTJGH3/p1596239699173000
I just ran my plugin through the new Block Checker and it's giving a Fatal Error with this reason: Found PHP call header(). This is likely to prevent your plugin from working as expected. It's referring to this code at the top of the plugin:
if (!defined('ABSPATH')) {
header('Status: 403 Forbidden');
header('HTTP/1.1 403 Forbidden');
exit();
}
It was in my understanding that this is always a good thing to put at the top of a plugin or theme in order to avoid direct access. Why would this be considered a Fatal Error ?
#62
in reply to:
↑ 61
@
4 years ago
Replying to JeffPaul:
I caught this question in the #core-coding-standards channel and wasn’t sure if this was an odd case we should consider with the checker?
Just noting that this was handled in #meta https://wordpress.slack.com/archives/C02QB8GMM/p1596240707221700
The suggestion was to not use the header()
calls but to keep the exit()
if wanted.
Allowing header()
to be used for other reasons would likely signify that a plugin really wasn't a Block Plugin so it really seems like banning it's use entirely is probably best.
#80
@
4 years ago
@tellyworth typo in https://meta.trac.wordpress.org/changeset/10213
...that attempts to insert the block %3%s into a post...
#82
@
3 years ago
TLDR: Can someone help check and see if there's a fix needed for the Block Plugin Checker for the apiVersion field?
Details:
Note that @richtabor may have found a logic error on the apiVersion check on the Block Plugin Checker as its throwing a block.json file error for his Markdown Comment Block plugin that the apiVersion
property must contain a string value. However the recent Block API Enhancements post seems to show this as an integer. I'm guessing this was a code copy mistake mimicking the same as the other version (string|false|null)
check.
#83
@
3 years ago
I opened PR against the Gutenberg repository that clarifies that apiVersion
in block.json
should be a number:
https://github.com/WordPress/gutenberg/pull/33249
All core blocks define apiVersion
with the number 2
. We also used number in all examples in documentation and in the initial dev note https://make.wordpress.org/core/2020/11/18/block-api-version-2/.
#84
@
3 years ago
Yesterday, in r11097, I increased the error level of block.json
issues, because most issues in that category will prevent block-plugins from appearing in the directory. The apiVersion
was always a string, but it was considered a recommended change before. I agree this is incorrect and should be updated to a number.
We need to define the block type schema to validate against it - is there a core-defined schema that we could use to avoid these kinds of mistakes? (I also asked this in slack)
@
3 years ago
Update apiVersion
to number, use rest_validate_value_from_schema
to validate json object
#85
@
3 years ago
It looks like I'd need to write a validate_number
function to add number support to the validator, so I've tried an different approach in 5303.diff - removing all our custom validation in favor of rest_validate_value_from_schema
. I think this does everything we want (and some, since it would support all the property definitions that the core schema validates), the only drawback is that it returns on the first invalid value, rather than listing them all.
#86
@
3 years ago
I like to submit that the schema no longer be in the plugin but rather pulled from https://json.schemastore.org/block.json or, once it's ready, the new package being proposed to manage the schemas for Gutenberg - https://github.com/WordPress/gutenberg/issues/35927.
I'd be happy to open a new ticket if that makes more sense.
#87
@
3 years ago
Thanks for sharing that issue, a @wordpress/schemas
package would be a great help here. Once that's released, we should use it here.
#88
@
3 years ago
https://schemas.wp.org/trunk/block.json is ready to use. I see that the block plugin checker shows three errors for the block scaffolded with the up-to-date local copy of npm @wordpress/create-block
which follows the block schema. I included the screenshot and I can also include the zip file if you want to use it for testing purposes. I'm concerned only about the issues reported for the block.json
file. The prompt for the unique namespace is a valid one and I appreciate including this check.
#90
@
3 years ago
In [11410], I removed all the custom schema checking for the remote schema & rest_validate_value_from_schema
, which almost works perfectly — the exception is a core bug #core54740. There's a workaround for it now, but hopefully the core issue can be fixed.
I did not cache the schema file locally, arguably it should be cached for a short time if this is called often. Any thoughts @tellyworth?
#91
@
3 years ago
This is awesome! I just tested it with one of the plugins from the examples repo and the JSON validated as expected.
One thing I noticed was that there are some existing blocks using the gutenberg-examples
namespace and even the full name of one of the example plugins gutenberg-examples/example-01-basic
is being used. Perhaps we can add some logic to flag those as being used?
#92
@
3 years ago
I just wanted to confirm that all the issue are resolved now when scaffolding blocks with npx wp-create-block esnext-test --namespace gutenberg --no-wp-scripts
in the Gutenberg repository. This is great news because it should continue to work correctly when we publish updates for WordPress packages to npm. Thank you for fixing it!
#94
@
3 years ago
- Resolution set to fixed
- Status changed from new to closed
I've added gutenberg-examples
to the "disallowed namespaces" list, so it will flag a warning like create-block/
does already.
With that, I'm going to close the ticket, as this has been up and running for a while now. If there are other issues with the block checker, they can be added as new tickets.
Initial UI improvements— some error message cleanup, restructured the flow of the results page, and removed the icon links for text links