[Patterns]: Featured patterns from pattern directory #35115
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ntsekouras
added
[Type] Enhancement
ntsekouras
requested review from
mcsf,
Mamaduka,
youknowriad,
ryelle,
mtias,
kjellr and
kellychoffman
ntsekouras
requested review from
ellatrix,
spacedmonkey and
TimothyBJacobs
as code owners
ntsekouras
commented
Sep 24, 2021
|
Size Change: +856 B (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
Mamaduka
reviewed
Sep 24, 2021
Mamaduka
reviewed
Sep 24, 2021
Mamaduka
reviewed
Sep 24, 2021
There was a problem hiding this comment.
Thanks for working on this, @ntsekouras.
The feature works as expected, including the missing image 😄
I would defer on the final decision to the folks who are more familiar with the patterns directory.
P.S. I found an unrelated issue while testing this. If you resize the browser window so that bottom is near the last pattern, it starts "glitching."
CleanShot.2021-09-24.at.18.17.44.mp4
|
Very cool! This seems to work well in my testing. @shaunandrews was there any talk about a "Featured" category in the patterns directory? @melchoyce, @beafialho and I can help curate a short list of patterns to start featuring here, but I wonder if it makes sense to have this list featured in the actual pattern directory or not at this point? |
ryelle
reviewed
Sep 28, 2021
lib/block-patterns.php
Outdated
Comment on lines
269
to
270
| // TODO: check to update the controller here: https://github.com/WordPress/gutenberg/blob/trunk/lib/class-wp-rest-pattern-directory-controller.php#L321 | ||
| // because it unsets the `per_page` param. |
There was a problem hiding this comment.
IIRC, pagination is disabled because there is no way to paginate in the UI, so we want to return every pattern from the API for local filtering. Is there a reason to request just 2 patterns here?
There was a problem hiding this comment.
I think this needs to change from unsetting entirely, to support the params and set defaults or make the other request to pass -1 to fetch all. The API needs to support other use cases as well besides the pattern directory UI.
Is there a reason to request just 2 patterns here?
2 was just for demo and I guess we will show a bit more - maybe 10 or something? I believe we have to have a limit though because of performance and the current UI for selecting patterns, which is not super friendly for previewing a large number of patterns. We can revisit of course later..
There was a problem hiding this comment.
Turns out I was misremembering slightly - the wp.org API does not support pagination, it always requests 100 patterns. If/when we revamp the pattern inserter, we can update the API to support pagination, and then we can reenable these params here. For now, the featured category is manually curated, so you can be sure it's a smaller list.
Yeah, we decided not to launch with it because the technical details were unclear. If it's just another pattern category, we can create that any time.
Can you identify which pattern this is? I'm not seeing a missing image. |
You can see the error in developer tools when you open the |
Ok cool. I think for now, it would make sense to just curate a short list of 10 or so patterns. We can get started on that later this week/early next. |
|
The new category, Featured, has been created - you can query it with |
ntsekouras
force-pushed
the
try/featured-patterns-from-pattern-directory
branch
from
1bb837b to
0abfd1f
Compare
|
Over in WordPress/pattern-directory#365, @beafialho, @melchoyce and I pulled together a collection of 12 initial patterns to feature. That should be a decent starting point for now. 👍 |
Awesome! So I guess this just needs a code review now 😄 |
ryelle
reviewed
Oct 5, 2021
lib/block-patterns.php
Outdated
Comment on lines
269
to
272
| $pattern_name = sanitize_title( $pattern['title'] ); | ||
| if ( ! WP_Block_Patterns_Registry::get_instance()->is_registered( $pattern_name ) ) { | ||
| register_block_pattern( $pattern_name, (array) $pattern ); | ||
| }; |
There was a problem hiding this comment.
Some of these same patterns are already being registered with the prefix core/ and there's no prefix here, so I'm seeing a few duplicates in the featured tab: core/heading and heading, core/large-header-with-left-aligned-text and large-header-with-left-aligned-text, etc.
There was a problem hiding this comment.
hm... I updated with a check if a pattern is registered with the core prefix. This made me wonder why we need the core prefix in the first place.. Should we also incorporate the id that exists in pattern directory in is_registered?
mcsf
approved these changes
Oct 5, 2021
lib/block-patterns.php
Outdated
Comment on lines
270
to
272
| if ( ! WP_Block_Patterns_Registry::get_instance()->is_registered( $pattern_name ) ) { | ||
| register_block_pattern( $pattern_name, (array) $pattern ); | ||
| }; |
There was a problem hiding this comment.
I'm noticing some things that are slightly tangent to this PR, but I'll still mention them:
- As in the previous function, the pattern's title is sanitised but the rest of the payload isn't, neither here nor in the patterns registry class. Seems like a code smell to me — either we are oversanitising or undersanitising.
- Though it's not the only case in WP, I've always found it very odd that the backend should make a HTTP request to itself. :) I don't know what arguments support this against just querying directly.
There was a problem hiding this comment.
Good questions Miguel. @ryelle does the sanitisation has to do with the current way the patterns are included in the directory? The inclusion happens manually for now so it's sanitised by 'us'?
I agree that such questions/changes can happen in a separate PR.
|
Great to see this! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is just a quick POC for fetching and showing
featuredpatterns from patterns directory.Resolves: #33046
Testing instructions
patternstabs and check the first shown category (featured) with patterns from the directory.Notes