Skip to content
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

Move the Block Patterns UI to the inserter #20951

Merged
merged 41 commits into from
Apr 17, 2020
Merged

Conversation

youknowriad
Copy link
Contributor

@youknowriad youknowriad commented Mar 17, 2020

Refs #17335

This PR is the second iteration on the block patterns UI. It moves the block patterns into the inserter.

Capture d’écran 2020-04-10 à 10 49 05 AM

It is only enabled for root-level insertions.

Potential follow-up tasks:

  • Support searching in the block patterns tab.
  • Disable patterns if one of the blocks is not allowed.
  • Allow inserting patterns inside container blocks that support it.

@youknowriad youknowriad added [Type] Enhancement A suggestion for improvement. Needs Design Feedback Needs general design feedback. Needs Accessibility Feedback Need input from accessibility [Feature] Patterns A collection of blocks that can be synced (previously reusable blocks) or unsynced labels Mar 17, 2020
@youknowriad youknowriad requested a review from ellatrix as a code owner March 17, 2020 10:26
@youknowriad youknowriad self-assigned this Mar 17, 2020
@@ -3,7 +3,6 @@
*/
import {
map,
pick,
Copy link
Contributor Author

@youknowriad youknowriad Mar 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code removed in this file is moved up to be shared with the patterns.

@mtias
Copy link
Member

mtias commented Mar 17, 2020

I don't think we should do this until we have a finalized design for the inserter as a whole.

@@ -99,6 +117,17 @@ $block-inserter-search-height: 38px;
}
}

// This extra div is needed because
// flex grow and overflow auto doesn't work well together.
.block-editor-inserter__scrollable {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I spent hours trying to make scrolling behave properly without this extra div without any luck. There's a mix between flex, auto-height popover computation, and scrolling that makes it hard to achieve. This solution is decent I think.

@github-actions
Copy link

github-actions bot commented Mar 17, 2020

Size Change: +1.01 kB (0%)

Total Size: 841 kB

Filename Size Change
build/a11y/index.js 1.02 kB +1 B
build/api-fetch/index.js 4.01 kB +1 B
build/block-directory/style.css 761 B +1 B
build/block-editor/index.js 105 kB +124 B (0%)
build/block-editor/style-rtl.css 10.2 kB -9 B (0%)
build/block-editor/style.css 10.2 kB -13 B (0%)
build/block-library/editor-rtl.css 7.08 kB -6 B (0%)
build/block-library/editor.css 7.09 kB -4 B (0%)
build/block-library/index.js 112 kB -4 B (0%)
build/block-serialization-default-parser/index.js 1.88 kB +2 B (0%)
build/blocks/index.js 57.7 kB +1 B
build/components/index.js 198 kB -11 B (0%)
build/components/style-rtl.css 16.9 kB +194 B (1%)
build/components/style.css 16.9 kB +197 B (1%)
build/compose/index.js 6.66 kB -1 B
build/core-data/index.js 11.2 kB +126 B (1%)
build/data/index.js 8.43 kB +1 B
build/date/index.js 5.47 kB -1 B
build/edit-post/index.js 27.9 kB +232 B (0%)
build/edit-post/style-rtl.css 12.3 kB -24 B (0%)
build/edit-post/style.css 12.3 kB -21 B (0%)
build/edit-site/index.js 10.5 kB +81 B (0%)
build/edit-site/style-rtl.css 5.05 kB +28 B (0%)
build/edit-site/style.css 5.05 kB +28 B (0%)
build/edit-widgets/index.js 7.49 kB +24 B (0%)
build/edit-widgets/style-rtl.css 4.67 kB +28 B (0%)
build/edit-widgets/style.css 4.67 kB +28 B (0%)
build/editor/index.js 43.3 kB +10 B (0%)
build/element/index.js 4.65 kB +2 B (0%)
build/i18n/index.js 3.56 kB -1 B
build/is-shallow-equal/index.js 711 B +1 B
build/keycodes/index.js 1.91 kB -1 B
build/notices/index.js 1.79 kB +1 B
build/priority-queue/index.js 789 B +1 B
build/rich-text/index.js 14.8 kB +1 B
build/token-list/index.js 1.28 kB +1 B
build/url/index.js 4.02 kB -2 B (0%)
build/viewport/index.js 1.84 kB -2 B (0%)
build/warning/index.js 1.14 kB +1 B
ℹ️ View Unchanged
Filename Size Change
build/annotations/index.js 3.62 kB 0 B
build/autop/index.js 2.82 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 6.24 kB 0 B
build/block-directory/style-rtl.css 760 B 0 B
build/block-library/style-rtl.css 7.17 kB 0 B
build/block-library/style.css 7.17 kB 0 B
build/block-library/theme-rtl.css 683 B 0 B
build/block-library/theme.css 685 B 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/data-controls/index.js 1.25 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 569 B 0 B
build/dom/index.js 3.1 kB 0 B
build/edit-navigation/index.js 3.54 kB 0 B
build/edit-navigation/style-rtl.css 485 B 0 B
build/edit-navigation/style.css 485 B 0 B
build/editor/editor-styles-rtl.css 428 B 0 B
build/editor/editor-styles.css 431 B 0 B
build/editor/style-rtl.css 3.48 kB 0 B
build/editor/style.css 3.47 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.32 kB 0 B
build/format-library/style-rtl.css 502 B 0 B
build/format-library/style.css 502 B 0 B
build/hooks/index.js 2.13 kB 0 B
build/html-entities/index.js 622 B 0 B
build/keyboard-shortcuts/index.js 2.51 kB 0 B
build/list-reusable-blocks/index.js 3.12 kB 0 B
build/list-reusable-blocks/style-rtl.css 226 B 0 B
build/list-reusable-blocks/style.css 226 B 0 B
build/media-utils/index.js 5.28 kB 0 B
build/nux/index.js 3.4 kB 0 B
build/nux/style-rtl.css 616 B 0 B
build/nux/style.css 613 B 0 B
build/plugins/index.js 2.67 kB 0 B
build/primitives/index.js 1.49 kB 0 B
build/redux-routine/index.js 2.84 kB 0 B
build/server-side-render/index.js 2.67 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

@enriquesanchez
Copy link
Contributor

Support searching in the block patterns tab

We had envisioned that the top search field will act as a global search, for both blocks and block patterns. Something like this:

Screen Shot 2020-03-17 at 15 19 16

Would that be technically feasible?

Disable patterns if one of the blocks is not allowed

Should disabled patterns not show up at all instead? I wonder about how to communicate why a pattern is disabled.

Allow inserting patterns inside container blocks that support it.

Do you mean containsers such as the group block? Are there any others you had in mind?

@youknowriad
Copy link
Contributor Author

Would that be technically feasible?

Yes, it's possible, the idea is to build things in steps as this is mentioned as a follow-up.

Should disabled patterns not show up at all instead? I wonder about how to communicate why a pattern is disabled.

When I say "disable", it doesn't necessarily mean a disabled button, it can be hidden.

Do you mean containsers such as the group block? Are there any others you had in mind?

Group, columns, cover, media & text and any third-party container block.


@mtias @enriquesanchez Let me know when you have a good design you want me to use as a target.

@mtias
Copy link
Member

mtias commented Mar 18, 2020

Allow inserting patterns inside container blocks that support it.

I've been thinking a bit about how this could be handled, particularly for template building / theme editor. Perhaps we need a core/pattern block that is basically just a placeholder to render pattern miniatures, or to force a pattern-only inserter to be used. (This would be similar to the "section" block discussed a long time ago.) I'd expect a common way to build templates would be to add and stitch together patterns.

@jasmussen
Copy link
Contributor

About this branch: this feels very promising, because I believe it's important that there remains only a single unified button to insert content. However I agree with Matías, it would be good to have a design ready before we go here.

One observation: clicking the Patterns tab takes a few milliseconds longer, presumably because it loads the block preview component that renders each block. This is with only 3 patterns present. It seems worth to:

a) try this with a bunch more patterns to see how much worse it gets, even if it's just duplicates of the same patterns
b) consider some way to asynchronously load each pattern, ie. a spinner inside each preview container so nothing is blocked as things load.

I've been thinking a bit about how this could be handled, particularly for template building / theme editor. Perhaps we need a core/pattern block that is basically just a placeholder to render pattern miniatures, or to force a pattern-only inserter to be used. (This would be similar to the "section" block discussed a long time ago.) I'd expect a common way to build templates would be to add and stitch together patterns.

The placeholder component is a powerful pattern for surfacing complex features in a simple-to-use way. In that vein, it seems like it could be a good way to surface patterns, which might otherwise "feel complex" when in fact it's supposed to be the opposite. If a section of the page shows thumbnails for you to pick from, it can be as little as one click to get something nice going.

But it does raise a few questions:

  • While we can make the placeholder element-query responsive now (and presumably collapse to a button that opens a modal) — would we want to limit such a pattern block to only the top level? Or an addon-question: how much sense does it make to insert a pattern as a nested block?
  • Patterns are intrinsically sections of a page, not a full page. Which means in order to build a whole page with just patterns, you need to insert several in succession.
    • Does that mean we insert multiple pattern placeholders in sequence to suggest this, and what would that look like?
    • Does it mean another placeholder gets inserted or suggested once you've inserted one pattern?

A placeholder for patterns really seems to make the most sense when inserted at the root level. In that vein, perhaps to truly enable page building with patterns, this suggests a rethink of the "trailing appender". This one:

Screenshot 2020-03-23 at 08 48 32

What if this interface defaulted and heavily leaned into suggesting patterns as the next section of the page. What would that look like? Would it default to inserting pattern placeholders, or would it simply show an interface to browse the patterns there?

@youknowriad
Copy link
Contributor Author

clicking the Patterns tab takes a few milliseconds longer, presumably because it loads the block preview component that renders each block. This is with only 3 patterns present

This is an issue regardless of the UI, we definitely need to find ways to improve this (even in the sidebar). I believe @retrofox @marekhrabe @obenland and friends already faced and explored solutions for this?

@youknowriad
Copy link
Contributor Author

how much sense does it make to insert a pattern as a nested block?

Inserting patterns inside blocks definitely make sense for me. Groups, columns, even post-content blocks

@jasmussen
Copy link
Contributor

jasmussen commented Mar 23, 2020

Groups, columns, even post-content blocks

To me those aren't patterns. Those are blocks.

I'm eternally open to be disagreed with and overruled here, but to me a pattern is multiple blocks that are customized and visually styled. Columns is an ingredient in that, not a pattern itself.

@youknowriad
Copy link
Contributor Author

@jasmussen Looking at the current patterns, I don't see why I won't be able to insert the "two columns of text" pattern inside a group block and apply some tweaks (background color, full wide,...) to the container

@jasmussen
Copy link
Contributor

Looking at the current patterns, I don't see why I won't be able to insert the "two columns of text" pattern inside a group block and apply some tweaks (background color, full wide,...) to the container

I don't disagree. I'm personally fine if we think patterns can be inserted at any nesting level. In the case of limiting the nesting level, I was primarily referring the theoretical "pattern block" that Matías suggested.

@mapk
Copy link
Contributor

mapk commented Mar 23, 2020

Here's a couple of my thoughts on this as well...

I also view patterns as a section of blocks. For me, this means a couple of things.

  1. When a pattern is added, I view it as a single unit since I added it as a unit. For this reason, I should be able to select the pattern and delete the whole thing as a single unit. This leads me to think every pattern should be in a container (ie. Group block).
  2. Because I've always imagined patterns as a "section of a page," I always thought they'd be more than just two columns of text, or two buttons side-by-side. Those are really just blocks that provide patterns. 😕 I imagined them as a complete section of a page – most likely something more complex. In this scenario patterns wouldn't be able to be nested inside other blocks from the gitgo. Now if a user wanted to drag it into another block once it's been added to the page, I could see allowing that, but not adding it directly inside a block. The Section block mentioned above is an exception.
  3. If patterns are now becoming something much more simplified than actual page sections (ie. two columns of text or two buttons side-by-side) then we should allow them to be added inside other blocks.
  4. I do prefer the ability to preview patterns from the Inserter and drop them in without a Pattern block first. I haven't tested this PR, but how does it differ from https://wordpress.org/plugins/design/ ? These seem to load quite speedily.

I'm open to any disagreements here. 😄 These notes are just my paradigm of patterns.

Do we need to define what patterns are further? Maybe something like, a Pattern must include 2 or more blocks.

@retrofox
Copy link
Contributor

clicking the Patterns tab takes a few milliseconds longer, presumably because it loads the block preview component that renders each block. This is with only 3 patterns present

This is an issue regardless of the UI, we definitely need to find ways to improve this (even in the sidebar). I believe @retrofox @marekhrabe @obenland and friends already faced and explored solutions for this?

Yeah, definitely something to improve. I haven't taken a look yet to this PR, but pretty sure it happens in other places where the <BlockPreview /> is used.

@obenland
Copy link
Member

[Previews taking a long time to load] is an issue regardless of the UI, we definitely need to find ways to improve this (even in the sidebar). I believe @retrofox @marekhrabe @obenland and friends already faced and explored solutions for this?

We faced it with the page layout selector on wp.com and worked around it by using static images instead. We traced it down to setState taking a long time but couldn't find a work around.

@mtias
Copy link
Member

mtias commented Mar 23, 2020

For the display of pattern previews we should apply a windowing technique.

@youknowriad
Copy link
Contributor Author

youknowriad commented Apr 1, 2020

Based on the last designs on #21080 It seems like this PR is moving toward the right direction.

For the sake of iteration, I'd like to propose that we move forward with the current state and continue the inserter iterations on small PRs to achieve the designs there. I believe the proposal is good enough and better than master (which mean mergeable state).

Unless you think we should do it all in one PR?

@youknowriad youknowriad requested a review from Soean as a code owner April 17, 2020 10:55
@youknowriad
Copy link
Contributor Author

@jasmussen I was not able to find a good fix for the "cropped thing", in fact I don't understand the issue yet :P can you help?

The patterns tab stop, yes, it's the iframe. I though @aduth fixed a related issue on the third-party library used there but maybe it's not updated yet?

Copy link
Member

@mtias mtias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's get this in and iterate, thanks for addressing all the issues raised so far.

@jasmussen
Copy link
Contributor

jasmussen commented Apr 17, 2020

Actually before you merge, I'm seeing one regression. The normal block toolbar is-pressed state appears affected negatively by changes made in this PR:

hover

It's probably just a background color applied on hover, or something like that.

Here's how it should look:

should

@youknowriad
Copy link
Contributor Author

youknowriad commented Apr 17, 2020

Going to revert the change about the auto-closing breakpoint. Our tests break spectacularly because of that, it seems there's no easy way to change the browser window size in Puppeteer at the moment so we're always considered in a big screen.

you can also have this right now which is not great

Capture d’écran 2020-04-17 à 1 27 32 PM

@youknowriad youknowriad merged commit 5494ff5 into master Apr 17, 2020
@youknowriad youknowriad deleted the update/patterns-ui branch April 17, 2020 13:03
@github-actions github-actions bot added this to the Gutenberg 8.0 milestone Apr 17, 2020
@simison
Copy link
Member

simison commented Apr 17, 2020

One thing to double-check is that it's not possible to render empty patterns menu by unregistering all patterns (#20867).

@Soean
Copy link
Member

Soean commented Apr 17, 2020

Wow, this is really great! 🎉

Short feedback:

  • Patterns title are not visible
  • Patterns should be searchable
  • Inserter tips should be dismissible
  • We need drag & drop from the inserter

@simison
Copy link
Member

simison commented Apr 17, 2020

Would it make sense to call "block patterns" just patterns? The fact they consist of blocks shouldn't matter much to user?

Screenshot 2020-04-17 at 19 14 47

@youknowriad
Copy link
Contributor Author

@simison that's already the case :) and I confirm that I added the logic to remove the tab when you unregister everything.

@Soean good feedback, the patterns title removal was a design choice (discussed a bit above)
we'll work on some follow-ups for the other points, feel free to try a PR for one of these if you're interested too :)

@justintadlock
Copy link
Contributor

On teams of writers, having the pattern title visible is helpful. One team member asks, "Which pattern is that you used in X post?" Another team member can tell them the actual title of the pattern, which can be seen in text and hopefully searchable. Just a scenario I've already seen come up.

@jasmussen
Copy link
Contributor

On teams of writers, having the pattern title visible is helpful. One team member asks, "Which pattern is that you used in X post?" Another team member can tell them the actual title of the pattern, which can be seen in text and hopefully searchable. Just a scenario I've already seen come up.

Totally valid, and easy to pull back. We can revisit this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Patterns A collection of blocks that can be synced (previously reusable blocks) or unsynced Needs Accessibility Feedback Need input from accessibility Needs Design Feedback Needs general design feedback. [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.