Skip to content
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.

Sign up for GitHub

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

edit-post: After closing welcome and content is empty, focus title field #40195

Merged
merged 7 commits into from
Apr 14, 2022
Merged

edit-post: After closing welcome and content is empty, focus title field #40195

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c4ddf1e
Posts: Make sure to focus title after welcome dialog close if empty.
alexstine Apr 8, 2022
534e15a
Use a ref to control focus on welcome close.
alexstine Apr 8, 2022
7fb70c4
Add E2E test coverage.
alexstine Apr 8, 2022
c2d4946
Code optimizing. Try to fix E2E tests.
alexstine Apr 9, 2022
b858bf8
Reviewer feedback.
alexstine Apr 12, 2022
d3ea07b
Move isCleanNewPost to edit-post useEffect.
alexstine Apr 12, 2022
929835f
Only pull initial value of isCleanNewPost.
alexstine Apr 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions packages/edit-post/src/components/visual-editor/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,12 @@ export default function VisualEditor( { styles } ) {
wrapperUniqueId: getCurrentPostId(),
};
}, [] );
const { isCleanNewPost } = useSelect( ( select ) => {
const { isCleanNewPost: _isCleanNewPost } = select( editorStore );
return {
isCleanNewPost: _isCleanNewPost(),
};
}, [] );
const hasMetaBoxes = useSelect(
( select ) => select( editPostStore ).hasMetaBoxes(),
[]
Expand Down Expand Up @@ -191,11 +197,11 @@ export default function VisualEditor( { styles } ) {

const titleRef = useRef();
useEffect( () => {
if ( isWelcomeGuideVisible ) {
if ( isWelcomeGuideVisible || ! isCleanNewPost ) {
return;
}
titleRef?.current?.focus();
}, [ isWelcomeGuideVisible ] );
}, [ isWelcomeGuideVisible, isCleanNewPost ] );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

  1. Ignore it with a comment.
  2. 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.
  3. Other ideas?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The useSelect call shouldn't be inside the callback but it should just replace the existing one line 116

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?


return (
<BlockTools
Expand Down
4 changes: 1 addition & 3 deletions packages/editor/src/components/post-title/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,9 +71,7 @@ function PostTitle( _, forwardedRef ) {

useImperativeHandle( forwardedRef, () => ( {
focus: () => {
if ( isCleanNewPost ) {
ref?.current?.focus();
}
ref?.current?.focus();
},
} ) );

Expand Down