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

FSE: Add block pattern infrastructure #40105

Merged
merged 13 commits into from
Mar 23, 2020
Merged

FSE: Add block pattern infrastructure #40105

merged 13 commits into from
Mar 23, 2020

Conversation

obenland
Copy link
Member

@obenland obenland commented Mar 12, 2020

Changes proposed in this Pull Request

  • Adds an FSE module to distribute block patterns.
  • Adds three block patterns as examples.

Testing instructions

  • Make sure to have the latest version of Gutenberg built.
  • Apply this PR.
  • Load the Editor and switch to "Block Patterns" in the toolbar.
  • Insert a block pattern.

TODO

  • Decide where to host images used in block patterns. headstartdata? As part of the plugin? Use Core's images?
  • Decide whether we need to support json files, given that we probably want to always localize pattern titles?
  • See how to best distribute translations. Not sure of the full-site-editing text domain even works?

@obenland obenland added [Type] Enhancement [Goal] Gutenberg Working towards full integration with Gutenberg labels Mar 12, 2020
@obenland obenland requested review from youknowriad, iamtakashi and a team March 12, 2020 22:03
@obenland obenland requested a review from a team as a code owner March 12, 2020 22:03
@matticbot
Copy link
Contributor

@obenland
Copy link
Member Author

/cc @mtias @ianstewart @davemart-in

@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello obenland! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D40292-code before merging this PR. Thank you!

@matticbot
Copy link
Contributor

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.

@davemart-in
Copy link
Contributor

@obenland should https://calypso.live/?branch=add/fse-block-patterns work in testing this? I tried it, but don't see the patterns button in the editor.

@obenland
Copy link
Member Author

FSE is using wp-calypso as a development environment only, it's not integrated with it.
With the docs in PCYsg-ly5-p2#wpcom-sandbox you can stream these changes from your local Calypso checkout to your sandbox and can test it that way

@mtias
Copy link
Member

mtias commented Mar 13, 2020

Core images are super limiting.

@lancewillett
Copy link
Contributor

Core images are super limiting.

Just to expand on this for clarity, what do you mean?

  1. Product quality — The image quality in the core image collection is poor. We should find a better source of high-quality images and host them in the plugin.
  2. Technical — The mechanism of hosting images on the w.org CDN is a limiting factor for performance, maintainability, other.
  3. Other?

With clarity on this, contributors like me can take clear next actions to help.

@ianstewart
Copy link
Contributor

@obenland do we want tracks events for the todo list?

@obenland
Copy link
Member Author

obenland commented Mar 16, 2020

It depends on what we want to track. I assume which pattern was inserted? Anything else?

@ianstewart
Copy link
Contributor

ianstewart commented Mar 16, 2020

Yep. Definitely which pattern was inserted. Can we track any other engagement with the patterns? Deletion and editing?

Can we track if the Pattern sidebar was opened?

@mtias
Copy link
Member

mtias commented Mar 16, 2020

@lancewillett the first! The ones that can be hosted / bundled there might be a more restrictive set than what could be used here. (I believe Unsplash was being used fine for WP.com.)

@mtias
Copy link
Member

mtias commented Mar 16, 2020

Can we track any other engagement with the patterns? Deletion and editing?

I don't think you can easily track this since patterns become just regular blocks once inserted. I'd only count insertion.

@obenland obenland added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Mar 16, 2020
@obenland
Copy link
Member Author

The patterns inserter surfaces the inserted blocks and a success notice, so to track patterns the best we could do would be to track success notices and then filter those? It wouldn't be very high quality data however. It would not account for translations and contain the entire notice text.

@amamujee
Copy link

amamujee commented Mar 17, 2020

Are we thinking of adding more detailed logging to blocks and patterns - e.g. inserted, deleted, modified @obenland @mtias ?

@obenland
Copy link
Member Author

Gutenberg is not built with tracking in mind, so we're rather limited in the ways we can track things.

These are the events we're currently tracking:

  • undo
  • redo
  • moveBlocksUp
  • moveBlocksDown
  • removeBlocks
  • removeBlock
  • moveBlockToPosition
  • deleteBlock
  • insertBlock
  • insertBlocks
  • replaceBlock
  • replaceBlocks
  • replaceInnerBlocks
  • createErrorNotice

We could track block modifications, I'm just not sure how useful that'll be. From what I was able to find out it looks like it fires for every keystroke when typing text, not just once after a block was updated.

@obenland
Copy link
Member Author

@ianstewart @iamtakashi When you get a chance, could you add the gutenberg-edge sticker to one of your test sites, apply D40292-code to your sandboxes, and test this PR on the site with the sticker? (and maybe on one without to verify nothing breaks :))
I'd also be interested in which other patterns you think we should ship with initially

@ianstewart
Copy link
Contributor

I'd also be interested in which other patterns you think we should ship with initially

We've started tracking that at …

https://github.com/Automattic/blockpatterns/issues?q=is%3Aissue+is%3Aopen+label%3A%22ready+to+ship%22

… with the Ready to Ship label in that repo.

@obenland
Copy link
Member Author

@ianstewart @iamtakashi I updated and added all patterns that are ready to ship.

These are the adjustments I made:

  • Replaced all html entities with their hex code
  • Removed all image id references to avoid unsuccessful API lookups
  • Unfolded all HTML for easier debugging later on (except coblocks gallery which breaks doing that)

Remaining blocker:

  • Decide where to host images used in block patterns.

@obenland
Copy link
Member Author

After a discussion with @iamtakashi and @ianstewart in p1584641583330800-dotcom-view-design-slack, we decided to create a dedicated pattern site for patterns and images to live. This will also allow us to switch to using json files easily, if we decide to do so at a later point in time.

@iamtakashi
Copy link
Contributor

@obenland Thanks for moving forward with a dedicated site approach. I am glad the process of adding and updating patterns is going to be easy. I appreciate it :bowing:

In the meantime, if I want to update some of the patterns that have already been added, what is the best way to do that? It's not urgent or anything, but I want to fix something in a pattern before the UI becomes available.

@marekhrabe
Copy link
Contributor

marekhrabe commented Mar 23, 2020

Following up with some internal conversations, it seems to be decided to use pbNZNv-I-p2 to store both patterns and their assets for hot-linking.

What the procedure to sync into here? self-answering: it seems to be very manual right now as I'm exploring the patterns and how certain parts were extracted into sprintf so they can be translated.

@iamtakashi Any updates on your required changes to patterns? Should I look for them somewhere in the dedicated site? I think we might need to act more quickly on this, as the Gutenberg update is coming to dotcom soon.

@iamtakashi
Copy link
Contributor

@iamtakashi Any updates on your required changes to patterns?

I was waiting for the instruction to update patterns :) Having them in the dedicated site would be the best now? If so I can move all the patterns to the site that are ready to ship this afternoon.

@simison
Copy link
Member

simison commented Mar 23, 2020

Related WordPress/gutenberg#21074

@marekhrabe
Copy link
Contributor

Having them in the dedicated site would be the best now? If so I can move all the patterns to the site that are ready to ship this afternoon.

From conversations I've been able to locate, the dedicated site seems like a way to go now. I or @obenland can port those patterns once ready over there. Some manual conversion work seems to be needed there from my understanding of the code

Copy link
Contributor

@marekhrabe marekhrabe left a comment

Choose a reason for hiding this comment

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

This is working well. I have tested with the latest Gutenberg from the plugin repo (7.7.1) which shows the pattern UI and as expected, it is filled with patterns as declared here. I have also tested with an older Gutenberg and there are no glitches or errors.

Technically, I think we are good to go on this PR. However, since we might soon get a finalized list of patterns and their assets loaded from our new designated location, I think we should either:

  • wait with this PR for the final content
  • remove content here and leave just the "two-columns" pattern and add other patterns separately.

Up to you to choose

Screenshot of the feature as a reference:

Screenshot 2020-03-23 at 17 39 07

@iamtakashi
Copy link
Contributor

Ok, I've added all the ready-to-ship patterns (11 patterns) to the master site now.

@ramonjd ramonjd mentioned this pull request Mar 23, 2020
23 tasks
@obenland obenland merged commit 781ceef into master Mar 23, 2020
@obenland obenland deleted the add/fse-block-patterns branch March 23, 2020 22:10
@obenland obenland removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Mar 23, 2020
@obenland
Copy link
Member Author

@iamtakashi I added all remaining patterns except "Subscription". The jetpack subscription block kept breaking in preview and on insert, so we'll have to revisit that one.

@iamtakashi
Copy link
Contributor

@obenland Weird. Can you open an issue with what you saw? It's probably worth to report here as the block seems to be worked on soon. Automattic/jetpack#15076

@ianstewart
Copy link
Contributor

ianstewart commented Mar 24, 2020

The "Call to Action" pattern looks a bit janky in the sidebar right now.

image

The preview is janky independent of the theme but I'm not sure at the moment if it needs a fix in Gutenberg or if it's on our end. If it's not a simple fix can we just remove it for now?

ianstewart added a commit that referenced this pull request Mar 24, 2020
Temporarily removing a broken-looking block pattern.

Ref:

#40105 (comment)
ianstewart added a commit that referenced this pull request Mar 24, 2020
Temporarily removing a broken-looking block pattern.

Ref:

#40105 (comment)
@ianstewart
Copy link
Contributor

to track patterns the best we could do would be to track success notices and then filter those? It wouldn't be very high quality data however. It would not account for translations and contain the entire notice text.

Until we have a better version of tracking would it be worthwhile/complicated to track this for EN only?

@obenland
Copy link
Member Author

It wouldn't be complicated, I'm just not sure how usable that data will be. It would track the pattern title, so patterns like "Call For Action" would not be distinguishable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Goal] Gutenberg Working towards full integration with Gutenberg [Type] Enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants