Display Block Information by matching block variations #27469
Conversation
|
Size Change: -888 B (0%) Total Size: 1.28 MB
ℹ️ View Unchanged
|
ntsekouras
force-pushed
the
try/variations-display-info-lower-level
branch
from
9e0381d to
e39ff8b
Compare
|
This is a comment of @talldan in another draft PR of mine for this problem and I'm just putting it here to have the discussions.
This is the issue - #25494 |
ntsekouras
force-pushed
the
try/variations-display-info-lower-level
branch
from
e39ff8b to
508c304
Compare
ntsekouras
changed the title
ntsekouras
added
[Feature] Block Variations
|
@talldan I think if this approach goes on, you could make use of
|
There was a problem hiding this comment.
I like that we're exploring this, but it's a delicate thing to get right. I've left some comments, but I still haven't decided which approach I prefer (e.g. new dedicated selectors, refactor base getBlockType selectors, let certain UI components manage it, etc.).
I also haven't commented on the extension of the Variations API with variationMatcher, but I'd like us to weigh it against different options (e.g. blockType.variationAttributeName, blockType.variations[0].isActive(), etc.).
|
Thanks for starting to look at it and sharing your feedback @gziolo and @mcsf - really appreciate it. This is a problem that we would need to solve (matching variation) and by discussing it, will find the best approach! I know that the current status of the PR (as of writing this comment) is not the best, but it's a start :) I'd like some feedback from others as well that have worked in related things. --cc @talldan, @youknowriad , @noahtallen |
ntsekouras
force-pushed
the
try/variations-display-info-lower-level
branch
2 times, most recently
from
09be23e to
faa27e5
Compare
I personally lean towards I was also wondering, whether the |
|
I have created a separate PR to refactor
I believe that the main use case of matching block variations doesn't need much customization per variation. I can't think of an example that could utilize that, but of course there could be... What seems to me important though is:
So I'm going to make it per variation. |
ntsekouras
force-pushed
the
try/variations-display-info-lower-level
branch
from
b880062 to
324d6a7
Compare
I removed the selector. I'll need to revisit |
ntsekouras
added
[Package] Block editor
packages/block-editor/src/components/use-matching-variation-information/index.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Nice work, I think it's close 👍🏻
There are some things to clarify:
- what should be the final API for
isActivematcher added to the block variation - how should
useMatchingVariationInformationbe called, the type proposed isWPBlockDisplayInformationso it needs some alignment, it feels likeuseBlockDisplayInformationwould better express intent - we should measure the performance implications of
useMatchingVariationInformation, at the moment it probably will recompute every timecore/block-editorchanges which would be good, we should ensure it doesn't re-render when other stores change.
ntsekouras
force-pushed
the
try/variations-display-info-lower-level
branch
from
2b880d7 to
1f83290
Compare
Changed like this per suggestion: #27469 (comment)
Makes sense and changed it to |
packages/block-editor/src/components/use-block-display-information/index.js
Outdated
Show resolved
Hide resolved
ntsekouras
force-pushed
the
try/variations-display-info-lower-level
branch
from
248db30 to
1114567
Compare
ntsekouras
force-pushed
the
try/variations-display-info-lower-level
branch
from
0c7a7c3 to
8501b34
Compare
There was a problem hiding this comment.
This PR is in excellent shape now so I left mostly nitpicks. Thank you for all iterations. I'm personally satisfied with the last revision proposed. I still need to test this functionality although I anticipate it being only a formal step because e2e tests added cover that 👍🏻
I will wait for your response before doing the final check and approve this PR.
packages/block-editor/src/components/use-block-display-information/README.md
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/use-block-display-information/README.md
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/use-block-display-information/README.md
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/use-block-display-information/README.md
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/use-block-display-information/README.md
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/use-block-display-information/index.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/use-block-display-information/index.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/use-block-display-information/index.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/use-block-display-information/index.js
Outdated
Show resolved
Hide resolved
Co-authored-by: Greg Ziółkowski <grzegorz@gziolo.pl>
Co-authored-by: Greg Ziółkowski <grzegorz@gziolo.pl>
Co-authored-by: Greg Ziółkowski <grzegorz@gziolo.pl>
Co-authored-by: Greg Ziółkowski <grzegorz@gziolo.pl>
|
Thanks for all the help here @gziolo! I addressed all the feedback and it's waiting for the final check to land 🎉 😄 |
gziolo
added
[Type] New API
|
There is one issue I discovered, the icon and its label in the block toolbar is coming from the block type rather than from the variation:
In all other places, I'm aware of, everything works perfectly fine. I did some testing with the profiler I couldn't notice any unnecessary re-renders in the block card when I was changing unrelated block attributes. Stats from the performance e2e tests look good as well:
|
Thanks for testing performance impact for this! 💯 @gziolo I have it mind and below is the reason. I will create a follow up as soon as this one lands. Sounds okay with you? From my PR description:
|
youknowriad
removed
the
Needs Dev Note
Description
This PR wants to solve the problem of displaying the proper information of a block, if that block had been created from a
block variation.This is achieved with:
useMatchingVariationInformation) which receives aclientIdand returns the appropriate block information to display.isActive) in Block Variation's settings. This optional property is a function to match block variations. It doesn't check dynamically all block properties to try to find a match, because in many cases it doesn't make sense. An example isembedblock where we can have changed theresponsiveattribute, so a match would not be found.The changes that have been made affect the
Block Card (Inspector Tools),Navigation List View (top bar)andBreadcrumbs.To make use of the new hook and handle
Transforms icon(in toolbar), I need to makeBlockSwitchera function component which is handled in this PR: #27674 to make reviewing easier.The icon in BlockSwitcher will be handled in a follow up after the refactoring PR lands.
Checklist: