-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Block Editor: Tips feature #22109
Block Editor: Tips feature #22109
Conversation
Totally related to #21080 |
Size Change: +1.44 kB (0%) Total Size: 823 kB
ℹ️ View Unchanged
|
Wondering if it's a good moment to add a Slot here. Do you have an opinion about this, @mtias? |
a: <a href="/customize.php?autofocus[section]=colors" target="_blank"/> | ||
} | ||
), | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I like the PR, I think most of these contexts and tips are WP-specific while the block-editor
package is not. (Imagine an editor used to write comments, or outside WP entirely).
We'd have to find a way to "register" these tips on "edit-post" maybe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what do you think about adding a slot there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel this is more something for a "registration APi". We also have an issue about the possibility to register block-specific tips, so it can scale to absorb that too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@davemart-in FYI, as this will increase the scope of #20196
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also have an issue about the possibility to register block-specific tips, so it can scale to absorb that too.
This one #17091?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree this should be on a register based API. It also needs to handle user permissions, since access to many of those admin views is not a given for all roles.
I wonder if we need to repeat the search query (i.e. theme) in the tip? |
Just fiddling with it, I think I would prefer to replace the random tip selection with something more deterministic - like always showing the first matched of something like that. It feels too busy to me when things change as you type. |
ec01471
to
4d07311
Compare
@youknowriad When you get a chance, would you mind taking this on another round of reviews? |
</div> | ||
</div> | ||
{ tip && ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding this here means potentially, the tip could show up in the inspector too. Did you really mean to add it to the BlockCard or more to the InserterPreviewPanel?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added it as an example of showing the scope of this approach, where it isn't limited only to the block inserter menu. There a handful of issues about Tips. Here, we'd like to show a block tip if its defined. This PR tackles it as well, allowing us registering tips by block type:
import { dispatch } from '@wordpress/data';
// Register Tips.
const { __experimentalRegisterBlockTip } = dispatch( 'core/block-editor' );
const tips = [
__( 'Add alternative text to your images to make them more accessible.' ),
__( 'Use a Cover block to add text on top of your image.' ),
__( 'To place two images side by side, convert them to a Gallery.' ),
];
tips.forEach( ( desc ) => __experimentalRegisterBlockTip('core/image', desc ) );
@@ -1,9 +1,11 @@ | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
useless empty line :P
@@ -8,6 +8,7 @@ import { | |||
getBlockType, | |||
} from '@wordpress/blocks'; | |||
import { __ } from '@wordpress/i18n'; | |||
import { select } from '@wordpress/data'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the usage of the global 'select' or 'dispatch' in Gutenberg should be limited as much as possible because they are singletons and not aware of the parent registries (nested contexts). They are also not reactive. So prefer useSelect
and useDispatch
instead.
keywords, | ||
description, | ||
}; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The more I think about this, the more I lean towards a dedicated package to register and retrieve tips across screens (and not in block-editor directly). A package similar to the keyboard-shortcuts one.
I'd like @aduth's opinion here too if possible.
It would also be cool to clarify the meaning of each argument (maybe using typing).
I assume:
- scope means the tips "location" (inserter here for example)
- context I don't know what it is
- keywords, potential keywords to filter the tips
- description (or content) is the actual content of the tip
Should we use an object in order to have a more extensible API?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The more I think about this, the more I lean towards a dedicated package to register and retrieve tips across screens (and not in block-editor directly). A package similar to the keyboard-shortcuts one.
Sounds good to me too as long as we agree that the Redux approach could fit well for the Tips feature.
Glad to move it to a dedicated package.
Should we use an object in order to have a more extensible API?
Probably, yes. Does it mean we could add arbitrary data into the state tree? Or maybe we could an extra
field in order to store additional data.
context I don't know what it is
It's a good example of additional data required only for some tips. In this case, I used it as a way to define tip context in the block inserter menu. Its values could be CSS
, header
, theme
, plugin
and color
. Not totally needed to be honest. We can get rid of it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could the @wordpress/notices
package serve as prior art for this sort of requirement? Notices are somewhat similar in being data which represents a text-like representation somewhere in the page. Notices were even modeled to support multiple "groupings" / "areas" where they could be assigned (context
), though we don't currently leverage it as far as I'm aware.
I wanted to share some feedback about the visibility of such tips. The tips may be very unlikely to be seen if one just clicks the + button without typing anything. In this scenario, they are at the very bottom of the very long list of blocks. It may be better to show them in the block inserter in the editor itself (just above or below block-inserter-low-visibility.mov |
Description
This PR tries to tackle how to deal with Tips in a comprehensive way, using the store in order to decentralize access to the API. The goals to achieve are:
Related issues:
API
Actions and selectors are prefixed with
__experimental
. All data is stored in thecore/block-editor
subtree, under thetips
key.Register Tips (Actions)
__experimentalRegisterBlockTip( scope, description )
When registers a tip we need to define its scope and description as a minimum. The simpler case is when we register a Tip for a block type. For instance...
It will populate the store where, in this case, the scope is the block type. We can store more than one tip per scope. For instance, adding three tips to
core/image
block:__experimentalRegisterBlockInserterTip( context, keywords, description )
It registers Tips related to the Block Inserter. In #20196, we'd like to show some contextual tips when the user types specific words, such as
CSS
,header
,theme
, etc. In this case, the scope is a constant value:blockInserter
. And also, we can define a set of keywords that will be used to pick up the tip. For instance, we do something like the following:Get Tips (Selectors)
__experimentalGetBlockInserterTipByContext( state, type, random );
Use this selector when you'd like to get all tips about a specific block type:
It will return the tip description, or
null
if there isn't any tip registered for the block type. Also, if there is more than one tip, and therandom
parameter istrue
, it will return one of those tips randomly. if random is false, it will return the first tip from the tips list.__experimentalGetBlockInserterTipsByContext( state, searchTerm, random )
Use this selector to get block inserter contextual tips. It will return a tip according to the given search term. It also supports the
random
mode whether there is more than one tip for the search term:Adding tips to the interface
Block Inserter
This PR registers five tips to the block inserter component, in
tips.js
file of the edit-post package. The following is one item of the tips array:The first element is the context used to register the tip. The second one defines the tip keywords, used to compare and show it where/when it's appropriated. And the third one is the Tip description. It worths noticing that it could be a component. In this case, we'd like to translate the tip description.
These tips are registered in the constructor of the Editor class.
In another hand, the block inserter will try to pick the tip when the user types a search term in the searcher input box. If it matches, it will show one of the registered tips.
This approach defines more than a keyword for the tip. in the case of the
css
, it will be shown with thestyle
term as well:Finally, we are translating some keywords in order to make it work in other languages. For instance, in the following screenshot I typed
cabecera
which is the translation ofheader
to Spanish:Block Preview
This PR also shows a tip in the block preview of the block inserter whether the block type has some tips registered. In this PR we've registered three tips for the
core/image
block, and one for thecore/video
block, picked from this list.Since
core/image
has three tips registered and the selector passrandom
astrue
, it will change the tip every time that the preview is re-rendered:How has this been tested?
TBD
Checklist: