edit-post: After closing welcome and content is empty, focus title field #40195
Conversation
alexstine
added
[Type] Enhancement
alexstine
requested review from
ajitbohra,
mkaz,
ryanwelcher and
juanmaguitar
as code owners
There was a problem hiding this comment.
@youknowriad @talldan I'm not sure who usually works on these files, any ideas?
| if ( isCleanNewPost && ( ! activeElement || body === activeElement ) ) { | ||
| if ( | ||
| isCleanNewPost && | ||
| ! isActive && |
There was a problem hiding this comment.
This will only fire if isActive is false.
| @@ -13,6 +13,8 @@ export const EDIT_MERGE_PROPERTIES = new Set( [ 'meta' ] ); | |||
| */ | |||
| export const STORE_NAME = 'core/editor'; | |||
|
|
|||
| export const EDIT_POST_STORE_NAME = 'core/edit-post'; | |||
There was a problem hiding this comment.
Is there a better way to do this? I can't import store from edit-post, I guess it's because edit-post imports editor store. Memory error when I tried.
| @@ -1089,19 +1090,44 @@ export function canUserUseUnfilteredHTML( state ) { | |||
| ] ); | |||
| } | |||
|
|
|||
| /** | |||
There was a problem hiding this comment.
I copied this from edit-post selectors.
| } | ||
| ); | ||
|
|
||
| /** |
There was a problem hiding this comment.
Not sure if there is a better way to do this but I require this value from edit-post store.
| * @return {boolean} Whether the pre-publish panel should be shown or not. | ||
| */ | ||
| export const isPublishSidebarEnabled = createRegistrySelector( |
There was a problem hiding this comment.
I refactored this to use isFeatureActive now that it is defined here.
|
Size Change: +3.02 kB (0%) Total Size: 1.23 MB
ℹ️ View Unchanged
|
| */ | ||
| export const isEditingTemplate = createRegistrySelector( | ||
| ( select ) => ( state ) => { | ||
| return !! select( EDIT_POST_STORE_NAME ).isEditingTemplate( state ); |
There was a problem hiding this comment.
The editPost store shouldn't be used in the editor package.
select( preferencesStore ).get( 'core/edit-post', feature ); this is also weird (I know it's not introduced in this PR) because it's fetching a preference of edit-post in editor package. cc @talldan
The reason it shouldn't be used is because it creates a cycle dependency. I'm not sure what you're trying to achieve here but you should probably move your logic to edit-post.
There was a problem hiding this comment.
@youknowriad When the welcome dialog opens and you select the Close button, focus just kind of gets lost in the void. The goal was to focus the post title field. I thought it might be easiest to fire the focus from the useEffect already defined for post title focus.
- Create a new post.
- Welcome dialog appears.
- Select Close button.
- Focus is placed in post title field because the post is empty.
This works fine when the welcome dialog does not appear.
There was a problem hiding this comment.
The PostTitle component though is a generic component that don't assume context (edit-post editor, template mode...). So either we find a generic heuristic and put it in the component or instead move the logic elsewhere (edit-post which has the context about everything)
There was a problem hiding this comment.
@youknowriad I used a different approach. Hopefully this looks better. Also added a new E2E test so you can see the expected behavior.
There was a problem hiding this comment.
select( preferencesStore ).get( 'core/edit-post', feature ); this is also weird (I know it's not introduced in this PR) because it's fetching a preference of edit-post in editor package. cc @talldan
It looks unusual, but I don't think select( preferencesStore ).get( 'core/editor', feature ) is right. The publish sidebar is part of the post editor and this piece of code says to get whether the post editor's sidebar is enabled. The core/edit-post part refers more to the application rather than the package. If a package name like 'core/editor' were used, this becomes seperate from the post editor's other preferences and it becomes hard to know what the preference is for, and whether is should apply to other editors.
Perhaps the issue is that the selector shouldn't be in the editor package, it could be deprecated here and moved to the post editor.
alexstine
added
[Package] Edit Post
| @@ -27,7 +33,7 @@ import { store as editorStore } from '../../store'; | |||
| */ | |||
| const REGEXP_NEWLINES = /[\r\n]+/g; | |||
|
|
|||
| export default function PostTitle() { | |||
| function PostTitle( _props, forwardedRef ) { | |||
There was a problem hiding this comment.
I read a post where using "_props" is okay in this context but not sure.
There was a problem hiding this comment.
I think since this component don't use any props, the argument is not used anyway, so you can name it whatever. In general I use _ as a name for situations like that but _props is fine too I guess.
| @@ -63,6 +69,14 @@ export default function PostTitle() { | |||
| }; | |||
| }, [] ); | |||
|
|
|||
| useImperativeHandle( forwardedRef, () => ( { | |||
| focusTitle: () => { | |||
| if ( ref.current && isCleanNewPost ) { | |||
There was a problem hiding this comment.
I make sure post is clean here to not requiring access to it before this function runs.
| @@ -85,16 +85,21 @@ function MaybeIframe( { | |||
| export default function VisualEditor( { styles } ) { | |||
| const { | |||
| deviceType, | |||
| isActive, | |||
There was a problem hiding this comment.
Should this be more something like isWelcomeGuideVisible?
| isEditingTemplate, | ||
| __experimentalGetPreviewDeviceType, | ||
| } = select( editPostStore ); | ||
| const { getCurrentPostId, getCurrentPostType } = select( editorStore ); | ||
| const _isTemplateMode = isEditingTemplate(); | ||
| const feature = _isTemplateMode | ||
| ? 'welcomeGuideTemplate' |
There was a problem hiding this comment.
If I'm not wrong the PostTitle component is not visible at all in the template mode (it's not rendered), meaning we should either change the element we focus on, or avoid focusing anything entirely.
There was a problem hiding this comment.
This will only happen for normal welcome guide now.
| @@ -63,6 +69,14 @@ export default function PostTitle() { | |||
| }; | |||
| }, [] ); | |||
|
|
|||
| useImperativeHandle( forwardedRef, () => ( { | |||
| focusTitle: () => { | |||
There was a problem hiding this comment.
Should this be just "focus"? it's already the "PostTitle" component so maybe focusTitle is a bit redundant
| @@ -63,6 +69,14 @@ export default function PostTitle() { | |||
| }; | |||
| }, [] ); | |||
|
|
|||
| useImperativeHandle( forwardedRef, () => ( { | |||
| focus: () => { | |||
| if ( isCleanNewPost ) { | |||
There was a problem hiding this comment.
I think this check should move outside of "post-title" component. Because it's not clear why "calling" ref.focus should abort if the post is new. So we can move this logic instead to https://github.com/WordPress/gutenberg/pull/40195/files#diff-175b6822ebac715bc984ae982f1d9230b5d1e441c98138d8a1fc7ccf275c19bcR194 (or remove it entirely)
There was a problem hiding this comment.
@youknowriad I thought it would be better to keep it there since it was already selected. Anyway, moved it to edit-post.
| return; | ||
| } | ||
| titleRef?.current?.focus(); | ||
| }, [ isWelcomeGuideVisible ] ); | ||
| }, [ isWelcomeGuideVisible, isCleanNewPost ] ); |
There was a problem hiding this comment.
A side effect of adding the value of isCleanNewPost as a dependency here is that the effect will run if isCleanNewPost changes even if isWelcomeGuideVisible didn't change. Meaning if the post was not clear but becomes clean (could happen programmatically for instance) while the welcome guide is open, the title will be focused and the welcome guide closes (focus moved outside).
The probably of the above happening is small of course but I think there's a better way to implement this logic. (Calling the selector inside the effect).
So you'd select the selector by doing :
const { isCleanNewPost } = useSelect( editorStore );
and then in the effect, you'd replace ! isCleanNewPost with ! isCleanNewPost() instead.
Note that this technique of calling selectors outside of useSelect should only be used if the returned value is not used in the rendering of the React tree but in an effect or event handler or any random callback.
There was a problem hiding this comment.
@youknowriad I get this error.
React Hook "useSelect" cannot be called inside a callback. React Hooks must be called in a React function component or a custom React Hook function.
What should I do?
- Ignore it with a comment.
- Add a comment explaining this effect will only fire if post is clean as defined in editor. Then not use isCleanNewPost in edit-post and leave that functionality in editor.
- Other ideas?
There was a problem hiding this comment.
Actually, I guess it will not work. I generated a local build and this was logged in console.
Uncaught Error: Invalid hook call. Hooks can only be called inside of the body of a function component. This could happen for one of the following reasons:
1. You might have mismatching versions of React and the renderer (such as React DOM)
2. You might be breaking the Rules of Hooks
3. You might have more than one copy of React in the same app
See https://reactjs.org/link/invalid-hook-call for tips about how to debug and fix this problem.
There was a problem hiding this comment.
The useSelect call shouldn't be inside the callback but it should just replace the existing one line 116
There was a problem hiding this comment.
Ah, okay. Fixed. Just the 1 call to useSelect( store_name ); retrieves once, yes? That way, once it is retrieved, it will not trigger the effect again?
What?
Fires the useEffect to focus post title after welcome dialog closes.
Why?
After closing welcome dialog, focus should be placed in a meaningful location.
How?
useEffect now listens for the preferences store feature changes.
Testing Instructions
Screenshots or screencast