Opened 7 months ago
Last modified 5 months ago
#5303 new task
Block plugin validation tool UI improvements
Reported by: |
|
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 (14)
Change History (95)
@
7 months ago
Allow passing in github branches, ex: https://github.com/ryelle/rmb-recipe-block/tree/try/block-json
#5
follow-up:
↓ 7
@
7 months 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
@
7 months 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
@
7 months 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.
7 months ago
#14
@
7 months 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
@
7 months 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
@
7 months 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
@
7 months 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
@
7 months 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
@
7 months 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
@
7 months 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
@
7 months 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
@
7 months 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.
@
7 months 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
@
7 months 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
@
7 months 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
@
7 months 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
@
6 months ago
I can confirm that Firefox mobile Version 27.0 (18428) correctly shows the triangle icons.
#40
@
6 months 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
@
6 months 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
@
6 months 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.
6 months ago
#44
@
6 months 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
@
6 months 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
@
6 months 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
@
6 months 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
@
6 months 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
@
6 months 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
@
6 months 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
@
6 months 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
@
6 months 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
@
5 months ago
@tellyworth typo in https://meta.trac.wordpress.org/changeset/10213
...that attempts to insert the block %3%s into a post...
Initial UI improvements— some error message cleanup, restructured the flow of the results page, and removed the icon links for text links