Modern Business: add FSE support #1062

vindl · 2019-07-13T20:09:22Z

Changes proposed in this Pull Request:

This adds required functionality to support rendering FSE header and
footer on all site pages (static and dynamic). It's also includes the
data population functionality, which has now been removed from the plugin
and will be delegated to each theme for maximum flexibility.

Testing instructions:

This PR requires its FSE counterpart PR to work properly. Please follow the testing instructions provided there: Automattic/wp-calypso#34640

apeatling · 2019-07-15T17:00:23Z

modern-business/inc/fse-template-data.php

+/**
+ * Class A8C_WP_Template_Data_Inserter
+ */
+class A8C_WP_Template_Data_Inserter {


What if instead of including all of the logic in the theme, you instead keep the logic in the plugin and expose helper functions that the theme calls to register template parts, and provide the default block template content (or block template name to request from the API)?

Even though we are using one base theme that all child themes inherit from, that may not always be the case, and you're opening the possibility of future copy-pasta. Abstracting away the hard work, but providing helper functions matches the general philosophy of WordPress and theming.

There's some discussion on this in the other PR, btw. :)

Automattic/wp-calypso#34640 (comment)

Ideally I'd like to do that and prevent the need for copy-pasta in the future. On the other hand I'm also trying to avoid posing the requirement that FSE plugin needs to be installed/active prior to theme activation - which might be problematic if the plugin code it relies on is missing.

Ideally I'd like FSE theme to be able to operate entirely independent of FSE plugin - there should be a fallback non-FSE header, and data population should work. That way, if the plugin is activated at a later time, FSE functionality will kick in immediately.

apeatling · 2019-07-15T17:00:45Z

modern-business/inc/fse-template-data.php

+	 *
+	 * @return string
+	 */
+	public function get_header_content() {


We talked about this in a call, but I think this data should be coming from an API.

Agreed, that would be a lot better than hardcoding it here. I don't see any blockers to doing that atm. 👍

mmtr

Great work here, @vindl! I definitely think this is the approach we should head to.

Everything is working according to the testing instructions when paired with Automattic/wp-calypso#34640

I also tested on a WP.com site and the data is now automatically populated after switching to the Modern Business theme. Since the data is automatically created, the issue noted in #34689 is not reproducible anymore.

Apart from the already mentioned suggestions (code duplication, content from API), I found some other minor things we maybe want to improve in follow-ups:

If I manually remove a template part, it is not created again automatically after switching back to Modern Business.
Since the data is created after switching to the theme, any existing site currently using the Modern Business theme won't have FSE functionality. Might not be an issue if we want to target only new sites.
For some reason, in my WP.com test site, the template/template parts has been created 5 times when switching to the theme. This doesn't affect the functionality, given we use the latest template created:

In WP.com, the template types are created with a pub prefix, as you can see on the screenshot above.

Of course, none of them is a blocker!

mmtr · 2019-07-17T05:23:42Z

modern-business/inc/fse-template-data.php

+	 */
+	public function get_header_content() {
+		// TODO: replace with header blocks once they are ready.
+		return '<!-- wp:group {"className":"site-header site-branding"} -->' .


I have not found any specific doc indicating how many Gutenberg versions do we support in FSE, but given that we still use the older @wordpress/editor package rather than @wordpress/block-editor (#), it seems that we want to support at least the 2 most recent WP versions (currently 5.1 and 5.2) like Jetpack does.

Since the group block is not present yet in the current WP version (#), should we avoid using it?

I think this part is required to make Modern Business header layout look as expected in the editor (cc @apeatling).

Since we are now targeting only Dotcom/Atomic, in the short term we can probably rely on the fact that we'll be running the latest version in both environments

vindl · 2019-07-17T10:51:11Z

Thanks for testing @mmtr!

If I manually remove a template part, it is not created again automatically after switching back to Modern Business.

I kind of relied here on the fact that template parts won't be exposed and hence deletable. I could improve it to check for entries based on taxonomy (instead of site option) and reinsert them when some part is missing.

Since the data is created after switching to the theme, any existing site currently using the Modern Business theme won't have FSE functionality. Might not be an issue if we want to target only new sites.

That's a good point - this depends on how we want to handle existing Modern Business sites (cc @apeatling @gwwar). In case we need to populate data for all such sites, we could make data population execute on after_setup_theme. This would effectively trigger it on each load, so we'll have to be very careful not to create duplicate entries.

For some reason, in my WP.com test site, the template/template parts has been created 5 times when switching to the theme.

Good catch! This is weird given that it's gated/marked by site option. Wondering if it's related to option caching or maybe the pub/ prefix.

jeraldjuice · 2019-07-17T23:08:18Z

Stumbled across something today - this triggers an error when doing the following:

Both the FSE plugin and Modern Theme are active on a site.
Deactivate the FSE plugin.
Try to activate the FSE plugin again. Failure because the Template Inserter class is already defined.

mmtr · 2019-07-18T01:47:54Z

Stumbled across something today - this triggers an error when doing the following:

Both the FSE plugin and Modern Theme are active on a site.

Deactivate the FSE plugin.

Try to activate the FSE plugin again. Failure because the Template Inserter class is already defined.

Seems this is caused by having the theme branch diverging from the calypso branch: Automattic/wp-calypso#34640 (comment)

mmtr

Seems that folks like this idea and no blockers have been identified that can’t be addressed in follow ups, so giving my 👍to this.

After landing this, I think we should update the documentation (readme/FG) to make clear that we need now to switch to the Modern Business theme to get the FSE functionality.

this depends on how we want to handle existing Modern Business sites

I think is ok for now to focus on sites switching to the theme.

This adds required functionality to support rendering FSE header and footer on all site pages (static and dynamic). It's also includes the data population functionality, which has now been removed from the plugin and will be delegated to each theme for maximum flexibility.

FSE contant will always be defined due to the fact that this plugin bundles other functionality (SPT). Because of that we can't rely on it to determine if this feature has been activated.

vindl · 2019-07-18T15:48:12Z

Stumbled across something today - this triggers an error when doing the following:
Both the FSE plugin and Modern Theme are active on a site.
Deactivate the FSE plugin.
Try to activate the FSE plugin again. Failure because the Template Inserter class is already defined.

Since the corresponding Calypso PR has been merged, this should no longer be the case. I'm assuming this was occurring while testing this PR with Calypso master that tried to declare that class to with old FSE plugin approach.

vindl · 2019-07-18T15:52:14Z

Lots of great feedback in the comments that I think should be incorporated. ✨
In order to prevent this PR from getting too bug and to keep things moving, I'm going to merge this as is for now. This should also help us parallelize some of the follow up improvements.

Thank you all for reviews and suggestions!

vindl self-assigned this Jul 13, 2019

vindl requested review from gwwar, noahtallen and apeatling July 13, 2019 20:09

vindl force-pushed the update/fse-modern-business-data branch from 2a8ccd1 to c9e32e2 Compare July 13, 2019 20:10

vindl mentioned this pull request Jul 13, 2019

Full Site Editing: render template parts on all site's pages Automattic/wp-calypso#34640

Merged

vindl requested review from obenland and jeraldjuice July 13, 2019 20:35

vindl mentioned this pull request Jul 13, 2019

FSE: The header and footer template parts need to render on dynamic pages on the front end Automattic/wp-calypso#34560

Closed

vindl force-pushed the update/fse-modern-business-data branch 2 times, most recently from b35ae77 to ea75607 Compare July 14, 2019 09:55

vindl requested review from shaunandrews and mtias July 15, 2019 15:06

apeatling reviewed Jul 15, 2019

View reviewed changes

mmtr reviewed Jul 17, 2019

View reviewed changes

mmtr mentioned this pull request Jul 18, 2019

Full Site Editing: new page WSOD on Simple Sites Automattic/wp-calypso#34689

Closed

mmtr approved these changes Jul 18, 2019

View reviewed changes

vindl added 2 commits July 18, 2019 17:42

Use more reliable check for FSE

ffcc2e3

FSE contant will always be defined due to the fact that this plugin bundles other functionality (SPT). Because of that we can't rely on it to determine if this feature has been activated.

vindl force-pushed the update/fse-modern-business-data branch from ea75607 to ffcc2e3 Compare July 18, 2019 15:42

vindl merged commit f046edf into master Jul 18, 2019

vindl deleted the update/fse-modern-business-data branch July 18, 2019 15:53

vindl mentioned this pull request Jul 18, 2019

Full Site Editor: populating our template_part header as part of site creation on WP.com [3] Automattic/wp-calypso#33515

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modern Business: add FSE support #1062

Modern Business: add FSE support #1062

vindl commented Jul 13, 2019 •

edited

Loading

apeatling Jul 15, 2019

jeraldjuice Jul 15, 2019

vindl Jul 15, 2019

apeatling Jul 15, 2019

vindl Jul 15, 2019 •

edited

Loading

mmtr left a comment

mmtr Jul 17, 2019

vindl Jul 17, 2019

vindl commented Jul 17, 2019

jeraldjuice commented Jul 17, 2019

mmtr commented Jul 18, 2019

mmtr left a comment

vindl commented Jul 18, 2019

vindl commented Jul 18, 2019

Modern Business: add FSE support #1062

Modern Business: add FSE support #1062

Conversation

vindl commented Jul 13, 2019 • edited Loading

Changes proposed in this Pull Request:

Testing instructions:

apeatling Jul 15, 2019

Choose a reason for hiding this comment

jeraldjuice Jul 15, 2019

Choose a reason for hiding this comment

vindl Jul 15, 2019

Choose a reason for hiding this comment

apeatling Jul 15, 2019

Choose a reason for hiding this comment

vindl Jul 15, 2019 • edited Loading

Choose a reason for hiding this comment

mmtr left a comment

Choose a reason for hiding this comment

mmtr Jul 17, 2019

Choose a reason for hiding this comment

vindl Jul 17, 2019

Choose a reason for hiding this comment

vindl commented Jul 17, 2019

jeraldjuice commented Jul 17, 2019

mmtr commented Jul 18, 2019

mmtr left a comment

Choose a reason for hiding this comment

vindl commented Jul 18, 2019

vindl commented Jul 18, 2019

vindl commented Jul 13, 2019 •

edited

Loading

vindl Jul 15, 2019 •

edited

Loading