Core: Adding block styles via editor.BlockEdit filter #23993
Closed
Conversation
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
|
Size Change: +229 B (0%) Total Size: 1.49 MB
ℹ️ View Unchanged
|
retrofox
changed the title
marekhrabe
reviewed
Jul 21, 2020
Contributor
marekhrabe
left a comment
marekhrabe
left a comment
There was a problem hiding this comment.
Described usage works as advertised! Styles are not hardcoded and now go in with a hook.
I have one tiny concern, outside of my scope of knowledge. Essentially, in a block-inspector/index.js, the BlockStyles are removed. This means they no longer show up at all.
To bring them back, a new file was created hooks/block-styles.js. It hooks into the right filter and adds them under the same conditions as before. This file is imported in hooks/index.js so it always loads together with other similar code. All good, in a browser.
What I have noticed is hooks/index.js has a also a native variant — index.native.js. This is where my knowledge falls short and there are two variants of what could happen:
- we have removed hardcoded styles from the
BlockInspectorand never brought them back, because thehooks/block-styles.jsis not imported fromhooks/index.native.js - styles work differently on mobile and they don't use
BlockInspector.
I have no idea which one is true, as I have no experience with the native side of the editor. However, considering that hooks/index.native.js imports almost everything that the standard hooks/index.js does, I think we might need to add the import too.
Any idea who can we ping to review this? It's probably super easy to confirm for anybody having the experience with mobile.
Member
|
Hey there 👋
That is correct, we don't use the
For this case is not needed to import it in the Thank you for double checking that it wouldn't break anything for mobile! |
Contributor
|
That's perfect, @geriux! Thanks for your confirmation! And with that… |
marekhrabe
approved these changes
Jul 22, 2020
youknowriad
reviewed
Jul 23, 2020
Contributor
|
No objections from here. |
retrofox
force-pushed
the
update/block-styles-inspector-controls
branch
3 times, most recently
from
5a8c690 to
467de0e
Compare
retrofox
force-pushed
the
update/block-styles-inspector-controls
branch
from
467de0e to
974134d
Compare
Contributor
|
@retrofox will you be looking to merge this? |
Contributor
Author
yeah, let me rebase it and then will try to ship it. Thanks for the reminder, Dave 🙇♂️ |
retrofox
force-pushed
the
update/block-styles-inspector-controls
branch
from
974134d to
e2f9d7a
Compare
Contributor
Author
|
Rebased and ready for a new review? cc @getdave |
Contributor
|
@retrofox I'll add this to my list to review on Friday. |
retrofox
force-pushed
the
update/block-styles-inspector-controls
branch
from
e2f9d7a to
32a6251
Compare
retrofox
added a commit
to Automattic/jetpack
that referenced
this pull request
Oct 26, 2020
* paid-blocks: change filter to add upgrade banner It changes the filter to add the banner to the paid blocks, replacing it with editor.BlockEdit. It will allow move the banner at the top im the lbock settings sidebar, one the core PR ships WordPress/gutenberg#23993 * paid-blocks: increase edit filter priority
retrofox
force-pushed
the
update/block-styles-inspector-controls
branch
from
2b7a6c5 to
21e9ea3
Compare
gziolo
added
the
[Feature] Extensibility
retrofox
force-pushed
the
update/block-styles-inspector-controls
branch
3 times, most recently
from
6bc8edc to
888de38
Compare
Member
|
I was about to create PR for this and then came across this one. Thanks, @retrofox. Is there a reason for using P.S. Let me know if I can help to get this PR merged. |
Contributor
Author
🙇♂️
No, actually let me update the PR. It needs a rebase, too.
Sounds like a nice plan. 🤝 |
retrofox
force-pushed
the
update/block-styles-inspector-controls
branch
from
888de38 to
45d75e2
Compare
Contributor
Author
Ready for a new 👁️ round, @Mamaduka 🙇♂️ |
Mamaduka
reviewed
Mar 15, 2021
Member
|
Hi, @retrofox Tested this, and it works as expected. |
retrofox
force-pushed
the
update/block-styles-inspector-controls
branch
from
45d75e2 to
acf9861
Compare
Contributor
Author
🤦 fixed!. Thanks :-) |
retrofox
force-pushed
the
update/block-styles-inspector-controls
branch
from
c470b0b to
b498308
Compare
getdave
force-pushed
the
update/block-styles-inspector-controls
branch
from
b498308 to
0b714fe
Compare
getdave
reviewed
Apr 23, 2021
Contributor
getdave
left a comment
getdave
left a comment
There was a problem hiding this comment.
Nice simple change which seems to work well 👍
I tested this by temporarily adding the following to the core/image block's edit.js file
const myEditComponent = ( OriginalBlockEdit ) => ( props ) => {
return (
<Fragment>
<InspectorControls>
<div>This is at the top!</div>
</InspectorControls>
<OriginalBlockEdit { ...props } />
</Fragment>
);
};
addFilter( 'editor.BlockEdit', 'core/image', myEditComponent, 20 );I saw the text This is at the top! appear above the block styles in the block sidebar.
I should be possible to write simple e2e tests for this by using the pattern shown in packages/e2e-tests/specs/editor/plugins/align-hook.test.js which activates a Plugin for testing. Inside the Plugin, you can do the addFilter and add some random text above/below the block styles using the hook. Then you can use e2e tests to assert on the presence of the text.
Might be over the top though.
Otherwise this seems to test well for me.
7 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
This PR inserts the
<BlockStyles />component to the Inspector Controls hooking a function to theeditor.BlockEditinstead of hardcoding. It doesn't add anything new to the API just an improvement in terms of functionality.It allows to handler properly the components inner the Inspector Controls via the hooks, since right now the BlockStyles locates always at the top and it isn't possible to deal with it, because, as I said before, it's hardcoded 😢 .
How has this been tested?
<BlockStyles />shows as usual for these blocks that have defined style variations.Screenshots
Types of changes
Checklist:
practices/inline-documentation-standards/javascript/ -->