Fix missing hook deps in distraction free mode #47611
Merged
Conversation
This file contains 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
getdave
commented
Jan 31, 2023
| @@ -44,7 +44,7 @@ function Header( { setEntitiesSavedStatesCallback } ) { | |||
| select( editPostStore ).isFeatureActive( 'distractionFree' ) && | |||
| isLargeViewport, | |||
| } ), | |||
| [] | |||
| [ isLargeViewport ] | |||
There was a problem hiding this comment.
Question. Do we want the other selectors to be re-run when the viewport changes? If not should we decouple this selector and run separately?
There was a problem hiding this comment.
I think it's fine isLargeViewport is not going to change very often.
getdave
commented
Jan 31, 2023
| documentLabel: postTypeLabel || _x( 'Document', 'noun' ), | ||
| }; | ||
| }, | ||
| [ isLargeViewport ] |
There was a problem hiding this comment.
Same question as for https://github.com/WordPress/gutenberg/pull/47611/files#r1091830264
|
@draganescu I'm unsure how to test this to ensure nothing has broken... |
1 task
|
Any reason for not calculating Something like this: const isDistractionFree = isDistractionFreeMode && isLargeViewportEdit: This isn't a blocker. |
|
Size Change: +178 B (0%) Total Size: 1.31 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in a2a1738. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4055794579
|
That would indeed eliminate the need to pass the dependency to the |
@Mamaduka I updated the PR along those lines. |
draganescu
reviewed
Jan 31, 2023
| @@ -127,6 +126,8 @@ function Layout( { styles } ) { | |||
| }; | |||
| }, [] ); | |||
|
|
|||
| const isDistractionFree = isDistractionFreeMode && isLargeViewport; | |||
There was a problem hiding this comment.
Meganit: I would do the naming the other way around, leave the select isDistractionFree as it corresponds to the feature and name the const isDistractionFreeMode.
There was a problem hiding this comment.
😆 😆 Will fix
Actually decided against this as the variable you already had outside the hook was isDistraction free. Therefore this PR will retain the existing nomenclature.
Mamaduka
added
General Interface
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?
Adds dependency to re-run selectors if viewport changes.
Why?
See review comments in #45591 (comment)
How?
Add
isLargeViewporttouseSelectdeps.Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast