register template part variations server side. #31761
Conversation
|
Size Change: -19.2 kB (-1%) Total Size: 1.3 MB
ℹ️ View Unchanged
|
Addison-Stavlo
added
[Block] Template Part
Addison-Stavlo
requested review from
ntsekouras,
vindl,
jeyip,
mattwiebe,
david-szabo97,
Copons,
creativecoder and
jameskoster
Addison-Stavlo
changed the title
Addison-Stavlo
removed
the
[Type] Bug
Addison-Stavlo
changed the title
There was a problem hiding this comment.
I think this is a more consistent way of handling the variations compared to #31772. I vote for this one 😄
| return { | ||
| ...variation, | ||
| ...( ! variation.isActive && { isActive } ), | ||
| ...( typeof variation.icon === 'string' && { |
There was a problem hiding this comment.
Should we check icons[ variation.icon ] || set the TP default icon? The typeof === 'string' can be omitted.
Also related to that, should we just create a mapper object that would contain the header and footer icons to avoid importing all the icons and would default to layout? --cc @gziolo
There was a problem hiding this comment.
That all makes sense.
typeof === 'string' can be omitted.
I was wondering if maybe there was a way someone could have registered it with an actual icon instead of an icon name, and to just double check if it was a simple string at this point before trying to overwrite it.
There was a problem hiding this comment.
should we just create a mapper object that would contain the header and footer icons to avoid importing all the icons and would default to layout?
Yeah, I'l go ahead and limit this to just importing the icons we expect for template parts at this time (header, footer, sidebar, and layout).
It would be neat if in the future we could enable registering custom icons with a template part area. Its no requirement at this point, but if instead of defining 'icon' => 'icon_name' on the php side we could define 'icon' => $some_icon. But I'm currently drawing a blank on how one might go about that.
There was a problem hiding this comment.
I was planning on exploring allowing icons to be set with svg strings, but the tricky part is finding a solution that will also work with react native
There was a problem hiding this comment.
@gwwar that would be neat! Im not too familiar with native's limitations there.
|
FWIW, this one isn't working for me whereas 31772 is. |
That's odd 🤔 . Have you run |
|
Yup, unfortunately that is the case. |
|
This is not working for me as well. The filters executes correctly, so the build side effect works as expected, but Though, the block seems to be registered correctly in the PHP side, and Apparently, the Navigation Link block is the same, but I have no idea if that's intended (as in: it's not supposed to have variations added this way?). cc @gwwar EDIT: I've tested this both as EDIT 2: either way, I like this approach much more than #31772! ✨ |
|
oof, so we like the approach that doesn't seem to be working for some folks. Im not sure why this filter would work for some and not others. 😕 |
It looks like the Navigation Link gets around this issue by having fallback variations defined in JS to register in the case that Since this filter is not behaving reliably and consistent, but the subscribe approach is, id say we stick with that at least for the bug fix. I don't see whats wrong with a |
@Copons - out of curiosity... does it start working for you if you specify an earlier priority on the |
@Addison-Stavlo Tried with I'm cool with going with the subscription approach, but I'd first ask the Navigation Link folks if they have any pointers. What I meant when I said that it's not working as well, is that NavLink's original Again, I'm not familiar with that block, but I think it's worthwhile to check if it's broken too, as the cause might be the same that breaks this for some of us. |
100% agree |
Interesting! I'm currently running 5.8-alpha-50507 on my local wp-env, so I guess I'm not seeing the variations as the patch containing the server support is not included yet in my version 🤔 |
|
This should work now with fallbacks for folks on earlier versions that don't support the server side registration. |
There was a problem hiding this comment.
This is working well for me now! ✨
@jameskoster would you mind trying again just to be 100% sure?
|
It works 🥳 |
Addison-Stavlo
added
the
[Type] Bug
* try ensuring block is registered first? * initially register variations on server side, patch isActive and icons in JS * format php * use yoda conditions, i must * fix import * document side effect * actually document it... * add fallback variations * add inserter scope
Description
This explores refactoring how we define template part block variations. Instead of the current approach (subscribing to the store and waiting various conditions to register the variations), here we register the variations on the server side and patch the
isActiveandiconproperties on the JS side with a filter. I am not sure whether this approach makes more sense than the last, but explored it while trying to debug issues regarding variations not showing up for some people (after more investigation #31772 fixes this issue without this refactor).How has this been tested?
run
npm run buildand verify variations show up and work as expected.Screenshots
Types of changes
Checklist:
*.native.jsfiles for terms that need renaming or removal).