Page List: Move the modal to its own file. #47922
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
ajlende
reviewed
Feb 14, 2023
Comment on lines
275
to
282
| <BlockControls group="other"> | ||
| <ToolbarButton title={ __( 'Edit' ) } onClick={ openModal }> | ||
| { __( 'Edit' ) } | ||
| </ToolbarButton> | ||
| </BlockControls> |
There was a problem hiding this comment.
If we want to keep the previous behavior, this should only be rendered if allowConvertToLinks is truthy.
Comment on lines
141
to
143
| const [ isModalOpen, setModalOpen ] = useState( false ); | ||
| const openModal = () => setModalOpen( true ); | ||
| const closeModal = () => setModalOpen( false ); |
There was a problem hiding this comment.
Moving the state in here instead of its own isolated component means the whole thing will need to rerender instead of just the modal when it opens.
In this specific place, the performance impact is probably going to be negligible especially with many of the complex data fetching and processing wrapped in a useMemo. But I see complex components like this a lot in gutenberg that I think could be done better, so it seemed worth a note.
There was a problem hiding this comment.
I moved the state back to the child and then used a useEffect to to pass the opening function back to the parent. This avoids the rerender but I'm not sure if it's an anitpattern?
The alternative I considered was using a redux store, but we don't have a store for the block library.
There was a problem hiding this comment.
I pushed up a commit with what I was trying to explain since I didn't do a very good job with English 😅
We can't move the ToolbarButton out because then we're limited to using the Modal from the toolbar which is against the goal of this refactoring. The modal stays separate for reuse, but the open/close controls still get their own component.
scruffian
force-pushed
the
refactor/page-list-modal
branch
from
99218d1 to
ea15f88
Compare
|
Size Change: +588 B (0%) Total Size: 1.33 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 131350d. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4186688100
|
draganescu
reviewed
Feb 15, 2023
| { allowConvertToLinks && ( | ||
| <ToolbarButton | ||
| title={ __( 'Edit' ) } | ||
| onClick={ () => openModal() } |
There was a problem hiding this comment.
I don't think this is a solid approach - openModal can remain null and be called in various circumstances. Plus any component using this modal would need this closure to "receive" an internal method of the modal.
The modal should be mounted (hence open) when it's needed and this is in this edit component (open and close modal are things the edit component does, the modal only needs to know if it should show up or not, and that only to have an onClose event, instead of rendering conditionally).
Not sure what is the correct way suggested in #47922 (comment) but passing props up is basically the opposite of how it should be, I think?
scruffian
force-pushed
the
refactor/page-list-modal
branch
from
8273c99 to
a43447f
Compare
scruffian
changed the title
|
OK I further simplified this to just move the modal to its own file for now, just to keep the code cleaner. |
draganescu
approved these changes
Feb 16, 2023
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 is broken out of #47748.
Why?
These changes move the modal to a separate file.
How?
Refactoring
Testing Instructions