Font Library: Use Interface datastore for modal state management #58350
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
t-hamano
added
[Type] Code Quality
t-hamano
changed the title
t-hamano
force-pushed
the
font-library/interface-store-modal
branch
2 times, most recently
from
4e158d5 to
7196f50
Compare
|
Size Change: -65 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
t-hamano
added
[Type] Enhancement
|
Flaky tests detected in 909b292. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7707092634
|
t-hamano
force-pushed
the
font-library/interface-store-modal
branch
from
7196f50 to
741ac46
Compare
t-hamano
force-pushed
the
font-library/interface-store-modal
branch
from
741ac46 to
909b292
Compare
t-hamano
changed the title
t-hamano
commented
Jan 30, 2024
Comment on lines
+41
to
+46
| <FontLibraryProvider> | ||
| <RouterProvider> | ||
| <Layout /> | ||
| <PluginArea onError={ onPluginAreaError } /> | ||
| </RouterProvider> | ||
| </FontLibraryProvider> |
There was a problem hiding this comment.
@youknowriad Can you let me know if there are any issues with adding a new provider to this level?
The Font Library shares context in the Typography panel of the Global Style sidebar and in the modal dialog. By moving the modal dialog here, I needed to add a provider at this level to continue sharing context.
Judging by performance tests, there doesn't seem to be any performance degradation.
There was a problem hiding this comment.
I'm curious what is this provider about?
There was a problem hiding this comment.
Took a look at it (very quickly), it seems like a random provider to share some props and things like that. I'm not entirely sure that we conceptually need a context at all for this. Can't we work on removing it entirely?
There was a problem hiding this comment.
Can't we work on removing it entirely?
Yes, I think most of the context here can be removed with proper refactoring.
However, I think there is one context that must be shared between the modal dialog and the typography panel. We need to dynamically switch the content of a modal dialog depending on whether I click on an icon in the typography panel or on any font. Is there a way to achieve this behavior without using a provider?
e19a757fd9fba2e533186da827df815e.mp4
There was a problem hiding this comment.
If we are to move the modal open/close state to the interface package, then yes, context is an option. But it feels like we could have some "initialState" argument that we can pass to openModal.
In other words, how do we do this in other modals? Or maybe we didn't have a use-case in other modals to not open a modal in its initial state.
--
Regardless of this discussion, and since we have the context today, I think it's fine to add it top-level with the caveat that we should move away from it. Conceptually edit-site/app/index should be seen as the future wp-admin index. It seems that adding random providers there is not a great way to achieve things. "Routes" need to be lazy loaded basically.
There was a problem hiding this comment.
Or maybe we didn't have a use-case in other modals to not open a modal in its initial state.
As far as I've researched, there doesn't seem to be any such use case. I found #54199, which is similar to this approach, but I don't know if this issue is intended to control the initial state of the modal.
Personally, I don't want to rush this PR as there are other things that could be improved regarding the Font Library.
Should we consider adding new arguments to openModal after removing the context as far as possible and then come back to this PR?
There was a problem hiding this comment.
Should we consider adding new arguments to openModal after removing the context as far as possible and then come back to this PR?
Yes, I'd like that ❤️
|
As seen in this discussion, refactoring (removing) the context is necessary to move this PR forward. I would like to close this PR and submit a new issue to address the problem. |
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.
Preparing for #54880 to add a command to open the Font Library modal
What?
This PR moves the state of the Font Library modal from the React state to the Interface datastore.
Why?
The current Font Library relies on React state to open modals. To control the state of this modal from anywhere outside of the component, we need to save that information to the datastore.
An example of this approach is seen in the Preferences modal. If you search the entire project for PREFERENCES_MODAL_NAME, you should be able to see examples of its implementation.
How?
The font library currently relies on the provider with very large contexts. Moved the provider to a higher level to cover the Font Library modal and the Global Styles font panel in one provider. At the same time, the datastore now manages whether a modal is open or not, or the modal open/close actions. You can control the Font Library modal by opening the Site Editor canvas and running the following code in the browser's console.
wp.data.dispatch('core/interface').openModal('edit-site/font-library')wp.data.dispatch('core/interface').closeModal()Testing Instructions
The behavior of the font library itself should remain unchanged.
Screenshots or screencast
040d889711166963b7b590c9f077e29f.mp4