-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
FSE: New Patterns #43817
FSE: New Patterns #43817
Conversation
Caution: This PR affects files in the FSE Plugin on WordPress.com D45735-code has been created so you can easily test it on your sandbox. See this FieldGuide page about developing in the FSE Plugin for more info: PCYsg-ly5-p2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Firstly, these patterns are looking very cool! I've noted a few PHPCS errors that are being thrown in the build.
I have a couple of concerns about adding this many new patterns in the current interface we have for displaying patterns. Some of the designs are quite tall, which doesn't feel like the best format for displaying them, and the scrollbar for the Patterns section selection is now very long, and slows down that part of the page with rendering everything at once. For example, here's how the Multi-column Text with Headline is looking on my test Atomic site:
Are we reaching a point where we need a new interface for browsing, filtering or selecting the patterns? (I know I've seen a few different mockups for it, but wasn't sure at what point we'll hit the need to improve the browsing of the patterns so that it really feels easy to use).
I haven't had a chance to investigate each of the block errors, but a fair few of the blocks are failing rendering in my Atomic site. E.g. List
, Three Quotes
, Heading and Text in Two Columns
, Multi-column Text with Headline
, Image and Text Mosaic
, Subscription
.
apps/full-site-editing/full-site-editing-plugin/block-patterns/patterns/images-02.php
Outdated
Show resolved
Hide resolved
apps/full-site-editing/full-site-editing-plugin/block-patterns/patterns/images.php
Outdated
Show resolved
Hide resolved
apps/full-site-editing/full-site-editing-plugin/block-patterns/patterns/three-quotes.php
Outdated
Show resolved
Hide resolved
The narrow default viewport size for the preview is making patterns with text a lot taller. Let's add |
The last commit should address the viewport and the non-standards code. Doesn't yet address any Atomic issues. |
There are some improvements on the way in core Gutenberg for surfacing patterns on inline search in the canvas and some designs out there for at least two column browsing. |
@iamtakashi any commonalities jumping out for you with this list? |
In my testing, the following patterns are broken in Simple sites too.
In addition to the above,
It might be something to do with the column block. The following patterns share the use of the column block
I'm not sure why |
Ok, the first problem is that the My best guess is that it's invalid syntax, terminating the printf flag but consuming the character in the process: Interestingly, printf does complain if you don't provide an argument for those invalid flags, but still probably not a very useful behavioural quirk. I had no idea about any of this - :TIL: Short version: If we
Which only misses |
The I've just pushed the change, as it seems the easiest way to communicate clearly here, but it needs testing in more scenarios. Let me know if you need to me to circle back to do that tomorrow. Out of interest, where did these There seems to be another intermittent issue where the list-02 does not receive the list items, which results in an error in the console and an empty example in the inserter, but the actual block when you click on it is just fine. It's a bit late here, so I'll come back to that tomorrow unless someone knows the story there. I'm also still seeing a bunch of console errors when opening up the inserter after applying the percent fix I mentioned above, so I won't push that until I've had a chance to look at them tomorrow. |
Correct. It's https://dotcompatterns.wordpress.com/. |
@@ -14,7 +14,7 @@ | |||
<!-- /wp:spacer --> | |||
|
|||
<!-- wp:paragraph {"style":{"typography":{"fontSize":72,"lineHeight":"0.9"}}} --> | |||
<p style="line-height:.9;font-size:72px;"><p style="line-height:.9;font-size:72px;"><strong>%1$s<br>%2$s<br>%3$s </strong><br><strong>%4$s<br>%5$s<br>%6$s<br>%7$s<br>%8$s<br>%9$s</strong></p></p> | |||
<p style="line-height:.9;font-size:72px;"></p><p style="line-height:.9;font-size:72px;"><strong>%1$s<br>%2$s<br>%3$s </strong><br><strong>%4$s<br>%5$s<br>%6$s<br>%7$s<br>%8$s<br>%9$s</strong></p><p></p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't this make two empty paragraphs and having three paragraphs in total?
The original markup was also weird though, it looks like a paragraph wrapped in a paragraph. And looking at the inline style, the paragraph tag <p style="line-height:.9;font-size:72px;">
has been duplicated in the markup for some reason?
The particular pattern was made in here: https://wordpress.com/block-editor/post/dotcompatterns.wordpress.com/938 I saw a duplicated paragraph tag in the markup. Not sure how it got there, but I cleaned it up. So if we update the markup in |
I spoke too soon. The duplicate tag comes back when I reopen the pattern. This: becomes this, and it seems like a bug. |
The notifications in the console and editor are triggered when the HTML regenerated by Gutenberg doesn’t match what’s on the page, so if there’s a bug in one of the component blocks, Gutenberg will complain if we fix it. What should be able to unit-test this, I’ll have a look at writing that. It will probably be quicker than the manual testing I was planning, and should catch this whole class of problems in future |
cc @ramonjd just in case any of the above turns out to be useful for the Patterns API work. |
There are still a number of phpcs lints, all of which seem to be fixable through phpcbf. I wonder why our pre-commit hook isn't doing that for us? 🤔 wp-calypso/bin/pre-commit-hook.js Lines 135 to 154 in 8e806f2
|
I'll rebase and fix those lints. |
85d28fb
to
7c5931a
Compare
This PR does not affect the size of JS and CSS bundles shipped to the user's browser. Generated by performance advisor bot at iscalypsofastyet.com. |
BTW what's the state of this PR? Does the block markup need more checking/fixing, or is it ready to go? |
This seems vaguely familar. Is this taken directly from the dotcompatterns site, or run through any additional tooling? Specifically something that involves an XML or HTML parser (block patterns API something something)? I seem to recall a problem with those parsers that would change multi-node markup ( cc/ @deBhal |
I missed this message on Monday, sorry @ockham. I'll need some help testing on Atomic sites but I'll test again on Simple and check the status there. If the patterns load w/o errors we can merge and ship! |
That looks a lot like what I was seeing with the DOMDocument parser, it will emit errors but those are usually being tossed in most cases and the parser just repairs the html the best it can. WordPress/gutenberg/pull/24645 or D48290-code may have fixed those for this case, if there is actually invalid html in the pattern source that probably needs to just be fixed. |
edb9e7d
to
642a75a
Compare
They are all looking good on local. Can someone help me out by testing the patterns? |
Adding 23 new patterns to the FSE Plugin.
Adding missing viewport size to improve display and correcting some whitespace, trailing commas, and non-standard code.
642a75a
to
2ee8767
Compare
I fixed that youtube bug and will deploy this once tests pass |
This PR should add 23 new patterns to the FSE Plugin.
This is most of the patterns from …
https://dotcompatterns.wordpress.com/2020/05/13/quote/
through to …
https://dotcompatterns.wordpress.com/2020/06/16/audio-player/
with a few exceptions where I encountered a bug or small known issue. (We'll get to them in a later PR or via another deployment method.)