Fix template part variations registration subscription.#31772
Closed
Addison-Stavlo wants to merge 3 commits intotrunkfrom
Closed
Fix template part variations registration subscription.#31772Addison-Stavlo wants to merge 3 commits intotrunkfrom
Addison-Stavlo wants to merge 3 commits intotrunkfrom
Conversation
Addison-Stavlo
added
[Type] Bug
Addison-Stavlo
requested review from
Copons,
creativecoder,
david-szabo97,
jeyip,
mattwiebe,
ntsekouras and
vindl
7 tasks
|
Size Change: +605 B (0%) Total Size: 1.32 MB
ℹ️ View Unchanged
|
gwwar
reviewed
May 12, 2021
Contributor
gwwar
left a comment
gwwar
left a comment
There was a problem hiding this comment.
import './variations'; is ignored by the non-dev build process as it does not see any use from the import. Instead we can import a function and call it from the code.
If there are side-effects, we can let webpack know by updating the sideEffects property:
gutenberg/packages/block-library/package.json
Lines 27 to 30 in ff4c3c4
https://webpack.js.org/guides/tree-shaking/#mark-the-file-as-side-effect-free
gwwar
reviewed
May 12, 2021
| const unsubscribe = subscribe( () => { | ||
| const definedVariations = select( | ||
| editorStore | ||
| ).__experimentalGetDefaultTemplatePartAreas(); |
Contributor
There was a problem hiding this comment.
I'm not sure if it'd be useful in this case, but just wanted to note it's possible to register variations via register_block_type_from_metadata eg
gutenberg/packages/block-library/src/navigation-link/index.php
Lines 325 to 331 in ff4c3c4
There is still some JS needed for icon setting/isActive logic, since there's a bit more API work to be done to help serialize that.
Contributor
Author
There was a problem hiding this comment.
Yes, thank you! I saw your PR registering them that way and it works great here. 😁
Contributor
Author
There was a problem hiding this comment.
oddly that approach doesn't seem to work with everyones testing. For some reason a couple folks seem to have that filter run before the actual php side registers the variations (?)- #31761 (comment)
Contributor
Contributor
Author
There was a problem hiding this comment.
That makes sense, thanks!
youknowriad
reviewed
May 12, 2021
| editorStore | ||
| ).__experimentalGetDefaultTemplatePartAreas(); | ||
| export default function subscribeToCreateVatiations() { | ||
| const unsubscribe = subscribe( () => { |
Contributor
There was a problem hiding this comment.
The global subscribe/select/dispatch is something we avoid using as much as we can. For what reason are we subscribing here? I assume we're waiting for something but what is this thing we're waiting for here?
(We should probably document this somewhere)
Contributor
Author
There was a problem hiding this comment.
Its waiting for the template part area definitions from the php side to initialize in the editor store.
Im looking at refactoring to avoid subscribe and register these variations from the server side here -#31761
Contributor
Author
There was a problem hiding this comment.
It looks like that fix is hit or miss for some folks, still have to investigate its inconsistencies.
For now, I would think this subscribe should be alright. Technically its only present until the store initializes.
Contributor
Author
There was a problem hiding this comment.
It looks like that fix is hit or miss for some folks, still have to investigate its inconsistencies.
Nm, that seems to be a WP version issue that we should be able to work around with fallbacks.
Member
|
@youknowriad would it be possible to include this fix in the 10.6 point release (if any)? |
Contributor
Author
|
closing this in favor of #31761 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes template part variations from not being present in non-dev builds.
import './variations';is ignored by the non-dev build process as it does not see any use from the import. Instead we can import a function and call it from the code.#31761 fixes this issue as well but also explores refactoring the registration to avoid the initial store subscription. At this point Im not sure if the refactor makes more sense or not, but I explored this refactor before figuring out the root cause of the bug.
How has this been tested?
run
npm run buildfrom this PR and verify variations ("Header", "Footer", in the block inserter) show up as expected.Screenshots
Types of changes
Checklist:
*.native.jsfiles for terms that need renaming or removal).