Skip to content
This repository has been archived by the owner on Feb 23, 2024. It is now read-only.

Explore if we can limit the Legacy Template block to only show in the block inserter on Woo block templates #5193

Closed
frontdevde opened this issue Nov 19, 2021 · 8 comments · Fixed by #5545 or #6539
Assignees
Labels
focus: FSE Work related to prepare WooCommerce for FSE. type: enhancement The issue is a request for an enhancement.

Comments

@frontdevde
Copy link
Contributor

For full context of the overall issue please take a look at #5180 first.

Following another round of testing of the 6.3.1 release, we decided to revert our previous decision to prevent the block deletion of the Legacy Template block, as this had the unintended side effect of limiting the user's creative ability to create compelling templates.

The goal of this issue is to explore (research spike) if there's a way to limit a block to only appear in the block inserter on specific block templates. It'd be the preferred solution for only showing the Legacy Template block in the block inserter on Woo block templates.

If that's not possible, it might potentially be easier to implement hiding the block from the inserter in the post/page editor but show it in the site editor.

@frontdevde frontdevde added the type: enhancement The issue is a request for an enhancement. label Nov 19, 2021
@nerrad nerrad added the focus: FSE Work related to prepare WooCommerce for FSE. label Dec 3, 2021
@sunyatasattva sunyatasattva self-assigned this Jan 6, 2022
@sunyatasattva
Copy link
Contributor

@frontdevde I did a bunch of research, here is what I found as a conversation starter:

1. Enabling a block in the inserter

This is, theoretically, as simple as setting: supports: { inserter: true }. However, when I enable it—even in the correct block templates—and I remove the old one and insert it again, the block doesn't show correctly neither in the editor, nor on the frontend.

Screen Shot 2022-01-06 at 18 35 29

This is because the legacy template block has been abstracted and its default properties as seen by the inserter are here:

https://github.com/woocommerce/woocommerce-gutenberg-products-block/blob/f2a2a3e51e69304acbbce33827c2f19a9d1df1a7/assets/js/blocks/legacy-template/index.tsx#L100-L103

While we hard-code the template type in the blocks themselves. See:

https://github.com/woocommerce/woocommerce-gutenberg-products-block/blob/f2a2a3e51e69304acbbce33827c2f19a9d1df1a7/templates/templates/archive-product.html#L3

https://github.com/woocommerce/woocommerce-gutenberg-products-block/blob/trunk/assets/js/blocks/legacy-template/constants.ts

So, at the moment, there is no front-end way to change that as that attribute is not exposed in the editor AFAIU. That means that, even if we solve the issue which is the focus of this research, we'll have to decide how to handle that: do we show a dropdown in the attribute sidebar? Do we show them as separate blocks in the inserter? I suppose we'd also have to limit which one we show depending on the location.

2. Conditionally enabling the inserter

I did scour around whatever we could have had available to do something like that. Initially, I was happy to find some useful data in the core/edit-site store, i.e.:

{
  editedPost: [{
    type: "wp_template",
    id: "tt1-blocks//taxonomy-product_cat",
    page: {
       context: {
         templateSlug: "taxonomy-product_cat"
       }
    }
  }]
}

Unfortunately, this data is not available at the time we register our blocks, but is hydrated a bit later through the SET_TEMPLATE action.

However, I noticed that SET_TEMPLATE does nothing more than getting that data from the URL itself, so we might as well do something like that in https://github.com/woocommerce/woocommerce-gutenberg-products-block/blob/trunk/assets/js/blocks/legacy-template/index.tsx:

import { getQueryArg } from '@wordpress/url';

const url = window.location.href;
const postType = getQueryArg( url, 'postType' );
const templateSlug = getQueryArg( url, 'postId' ).split( '//' )[ 1 ];

const ALLOWED_LEGACY_TEMPLATES = [ 'archive-product', 'single-product', /* ... */ ];

const isAllowedInInserter = postType === 'wp_template' && ALLOWED_LEGACY_TEMPLATES.includes( templateSlug );

// ...

registerBlockType( 'woocommerce/legacy-template', {
  supports: {
     inserter: isAllowedInInserter,
  }
});

Of course, this still means we'd have to take in consideration what's mentioned in (1.) above.

3. Alternatives

Other than this, I couldn't find any other data which is already exposed either in any data store, nor in globals which I know of that's any useful to determine where the user is within the editor.

However, if the URL approach above is not deemed solid enough, we could expose some useful data from the server through the AssetDataRegistry like it's done many times in this file, for example:

https://github.com/woocommerce/woocommerce-gutenberg-products-block/blob/b56790a52c41730de4fba25bb1640b6a0d7c46c0/src/BlockTypes/Checkout.php#L131

In this way we can definitely find the best and most solid approach.

Conclusion

As far as I understand, the goal of this issue was more the research spike and exploration itself than the implementation. If that's the case, I'd say the answer to the question posed in the OP is: yes. There are ways in which we could limit how legacy blocks show up in the inserter depending on where the user is. None of them are a one-liner, though.

The follow-up questions would then be:

  1. Given that this is bound to be a temporary fix until legacy templates are completely phased out, how many resources do we want to commit for this implementation?
  2. Do we want to go the quicker-and-dirtier route, or the more solid but a bit slower?
  3. Do we even want to do this or is this too niche?
  4. How do we want to solve (1.) from an architecture/design standpoint?

@frontdevde
Copy link
Contributor Author

frontdevde commented Jan 11, 2022

Thanks for looking into this, @sunyatasattva!

As far as I understand, the goal of this issue was more the research spike and exploration itself than the implementation.

That is correct!

That means that, even if we solve the issue which is the focus of this research, we'll have to decide how to handle that: do we show a dropdown in the attribute sidebar? Do we show them as separate blocks in the inserter? I suppose we'd also have to limit which one we show depending on the location. [...] How do we want to solve (1.) from an architecture/design standpoint?

Operating under the assumption that we would want users to only be able to use a block template that is relevant to their context, showing them a dropdown with all options is probably not ideal. So we'd want to look into the latter.

Note: if the above assumption holds true, we'd want users to only have access to the one block template that is relevant to their current context. This would mean regardless of what shows in the inserter, it would have to show the context-relevant block template upon insertion.

Thinking out loud here, maybe this could be achieved by registering a block variation for each template and only surfacing the context-relevant variation in the inserter. If I understand you correctly you're already thinking along these lines.

Given that this is bound to be a temporary fix until legacy templates are completely phased out, how many resources do we want to commit for this implementation? [...] Do we even want to do this or is this too niche?

Nothing more permanent than temporary solutions, right? 🙂 Jokes aside though, if there were a quick and clean solution, I'd argue for implementing it. If we feel like there isn't really, we should hold off to see how much user feedback we get of this being an actual problem. Based on what you found, if you had to make a call, what would you recommend?

@tjcafferkey
Copy link
Contributor

Great work investigating and presenting us all of the above options @sunyatasattva!

Thinking out loud here, maybe this could be achieved by registering a block variation for each template and only surfacing the context-relevant variation in the inserter. If I understand you correctly you're already thinking along these lines.

If we have the ability to do this, we could further reduce the scope of the work by leaving the Legacy Template block as-is and once inserted we then immediately set the template attribute depending on its context. This means we don't have to do any filtering on blocks in the inserter to ensure only the correct variation is present.

@sunyatasattva
Copy link
Contributor

Thanks both of you for the replies!

Based on what you found, if you had to make a call, what would you recommend?

I'd probably go with the fastest-to-implement solution. Adding more fine-tuned data from the server seems prone to polluting the scope and easy to forget when we move on.

The solution proposed by @tjcafferkey is my favorite: we show one block in the template and it automatically gets the correct attribute upon insertion. I wouldn't know exactly how to react to insertion, however, so I'd appreciate some guidance/pairing there if I have to implement it.

Otherwise, surfacing only the appropriate one in the inserter seems also doable and I might have a clue on how to make it work :)

@tjcafferkey
Copy link
Contributor

I wouldn't know exactly how to react to insertion, however, so I'd appreciate some guidance/pairing there if I have to implement it.

We could use useEffect without dependencies which will run when the component mounts in the editor (ie. has been inserted).

@tjcafferkey
Copy link
Contributor

tjcafferkey commented Jan 11, 2022

Here is a proof of concept of my idea but we would need to limit the legacy template block to only show up on the inserter for Woo templates.

#5545

Edit: I've added a bit of a hack for the inserter issue mentioned above. It works but I am open to better work arounds for that part of the problem.

@tjcafferkey
Copy link
Contributor

Reopening this issue as we had to revert the PR that closed this #5643

@tjcafferkey tjcafferkey reopened this Jan 27, 2022
@tjcafferkey tjcafferkey removed their assignment Jan 28, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Apr 1, 2022

This issue has been marked as stale because it has not seen any activity within the past 60 days. Our team uses this tool to help surface issues for review. If you are the author of the issue there's no need to comment as it will be looked at.

Internal: After 10 days with no activity this issue will be automatically be closed.

@github-actions github-actions bot added the status: stale Stale issues and PRs have had no updates for 60 days. label Apr 1, 2022
@sunyatasattva sunyatasattva removed the status: stale Stale issues and PRs have had no updates for 60 days. label Apr 1, 2022
@albarin albarin self-assigned this May 31, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
focus: FSE Work related to prepare WooCommerce for FSE. type: enhancement The issue is a request for an enhancement.
Projects
None yet
5 participants