Edit Site: Move loading logic to a separate hook #50511
Merged
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
tyxla
added
[Type] Enhancement
tyxla
requested review from
Mamaduka,
flootr,
jsnajdr,
ntsekouras and
youknowriad
|
Size Change: +4 B (0%) Total Size: 1.38 MB
ℹ️ View Unchanged
|
youknowriad
approved these changes
May 10, 2023
Contributor
youknowriad
left a comment
youknowriad
left a comment
There was a problem hiding this comment.
LGTM 👍 Thanks for the follow-up.
|
Flaky tests detected in b92c69b. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4938075397
|
jsnajdr
reviewed
May 10, 2023
tyxla
force-pushed
the
move/site-editor-loading-hook
branch
from
54176ac to
b92c69b
Compare
jsnajdr
approved these changes
May 10, 2023
Member
jsnajdr
left a comment
jsnajdr
left a comment
There was a problem hiding this comment.
Looks good now, I'm suggesting one more little improvement and that should be all.
| return { | ||
| hasResolvingSelectors: select( coreStore ).hasResolvingSelectors(), | ||
| }; | ||
| } ); |
Member
There was a problem hiding this comment.
One more optimization opportunity: this useSelect will return a new value every time any selector starts or stops resolving, even long after the editor is fully loaded. That will trigger a rerender of the Editor component that uses the useIsSiteEditorLoading hook.
You can make the return value more stable by including the loaded value as dependency:
const [ loaded, setLoaded ] = useState( false );
const inLoadingPause = useSelect( ( select ) => {
const hasResolvingSelectors = select( coreStore ).hasResolvingSelectors();
return ! loaded && ! hasResolvingSelectors;
}, [ loaded ] );This will guarantee a stable false return value once the loaded state is achieved. No more renderers.
Member
Author
There was a problem hiding this comment.
Good catch, I'll follow up in another PR.
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.
What?
This PR is a follow-up to #50222 (comment) and moves the site editor loading logic to a separate hook.
Why?
Helps abstract the loading logic from the
Editorcomponent and improve readability.How?
We're moving the logic to a hook and keeping the logic unchanged.
Testing Instructions
Same as testing instructions of #50222.
Testing Instructions for Keyboard
None
Screenshots or screencast
None