Extract utilities to @wordpress/editor and @wordpress/router
#63849
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
…rdpress/router as private APIs
oandregal
commented
Jul 23, 2024
| @@ -62,6 +62,7 @@ | |||
| "@wordpress/private-apis": "file:../private-apis", | |||
| "@wordpress/reusable-blocks": "file:../reusable-blocks", | |||
| "@wordpress/rich-text": "file:../rich-text", | |||
| "@wordpress/router": "file:../router", | |||
There was a problem hiding this comment.
This is the major concern I have to land this PR.
By doing this, the @wordpress/editor package would depend on some utilities provided by the router that require a <RouterProvider> in the component tree. Is this something the editor package cares about? Or is it the higher-up edit-site/edit-post/etc. responsibility instead? @youknowriad I presume you have thoughts about this and can share advice.
There was a problem hiding this comment.
This is a bit of a gray area.
If I understand properly, you want to move fields to "editor" (or core-data) or something like that. That's totally fine by me assuming all the fields are independent of the place they are used in (post editor, site editor...).
The problem is that the urls them selves are specific, in the site editor you want to navigate to some places but not in the post editor... Maybe for a start we can use always the same url and it should work regardless of where you are.
The other problem is that I'm not sure "urls" make sense in fields at all. I think for instance rendering "Media" or "Title" field or something shouldn't actually render an application url/link automatically. That feels like a separate config that should be passed to DataViews. For instance a getItemLink just like we have getItemId.
Also nothing that we already have something like getItemLabel but it's not implemented properly, right now we guess it by fetching the "primary" field which IMO is mixing separate concerns.
There was a problem hiding this comment.
If I understand properly, you want to move fields to "editor" (or core-data) or something like that.
Yeah. One of the issues I want to fix is: how do we share the field definition? Fields need to be available in three places now: edit-site/post-edit, edit-site/post-list, editor/post-actions.
There was a problem hiding this comment.
Yes, just like the actions.
My goal for the actions was to move them one by one (and type them) into editor/src/dataviews/actions and I can see the same for fields in editor/src/dataviews/fields.
I think ultimately, it makes sense for editor/src/dataviews to be its own package: core-dataviews or something or actually be part of core-data
youknowriad
reviewed
Jul 23, 2024
|
Size Change: +649 B (+0.04%) Total Size: 1.76 MB
ℹ️ View Unchanged
|
youknowriad
reviewed
Jul 23, 2024
| @@ -483,7 +476,13 @@ const viewPostAction = { | |||
| }, | |||
| callback( posts, { onActionPerformed } ) { | |||
| const post = posts[ 0 ]; | |||
| window.open( post.link, '_blank' ); | |||
| let url = post.link; | |||
| if ( isPreviewingTheme() ) { | |||
There was a problem hiding this comment.
Is this action, the reason you had to move previewing theme state to router?
There was a problem hiding this comment.
Also because the useLink/Link components have them as a dependency. If I move those to the @wordpress/router, it cannot depend on @wordpress/edit-site (where isPreviewingTheme/etc. would live). See what I know about the wp_theme_preview parameter at #63849 (comment)
There was a problem hiding this comment.
Maybe in addition to the idea I shared here #63849 (comment) the site editor can override this action.
youknowriad
reviewed
Jul 23, 2024
oandregal
commented
Jul 23, 2024
| @@ -33,6 +30,9 @@ export function useLink( params, state, shouldReplace = false ) { | |||
| ...Object.keys( currentArgs ) | |||
| ); | |||
|
|
|||
| // TODO: | |||
There was a problem hiding this comment.
The existing theme preview functionality has been introduced at #50030 and works this way: you can add a wp_theme_preview=THEME URL parameter and the new theme will be previewed. It works in the site editor, post editor, and front-end.
To make sure all links respect the existing wp_theme_preview parameter, navigation & links should use useLink or Link utilities. Some thoughts:
-
We need to account for the
wp_theme_previewparameter in more places than the site editor. For example, post actions, such as "View" (takes you to front-end) should respect that parameter. These actions are defined in the@wordpress/editorpackage so they can be shared between edit-post & edit-site. How do we go about making this work? This PR is a potential approach, but I'd like to double-check if it's sensible to go this way (see). -
@jsnajdr is it problematic that the params maintain the
wp_theme_previewbut it's not present in the history? Haven't been able to dig down into the router/history behavior.
There was a problem hiding this comment.
The initial goal of @wordpress/router was to be app agnostic and adding the "theme preview" logic into it breaks that.
Could we instead have a way to inject this logic as a "middleware" or something when rendering RouterProvider. In other words a prop to RouterProvider that changes how links work a bit (apply the them preview logic)
oandregal
commented
Jul 23, 2024
| } from '../../utils/constants'; | ||
| import { default as Link, useLink } from '../routes/link'; | ||
| import Media from '../media'; | ||
| import { unlock } from '../../lock-unlock'; |
There was a problem hiding this comment.
This is the gist of this PR. To be able to move post-fields from edit-site to the editor package, we need to remove any edit-site dependency:
- DataViews-related constants: make them inline.
- The
Mediacomponent: make it inline, as it was only used by post-fields. - Link, useLink: move to
@wordpress/routerpackage.
oandregal
commented
Jul 23, 2024
| @@ -483,7 +476,13 @@ const viewPostAction = { | |||
| }, | |||
| callback( posts, { onActionPerformed } ) { | |||
| const post = posts[ 0 ]; | |||
| window.open( post.link, '_blank' ); | |||
| let url = post.link; | |||
| if ( isPreviewingTheme() ) { | |||
There was a problem hiding this comment.
See conversation at https://github.com/WordPress/gutenberg/pull/63849/files#r1687870181
oandregal
added
[Status] In Progress
|
Warning: Type of PR label mismatch To merge this PR, it requires exactly 1 label indicating the type of PR. Other labels are optional and not being checked here.
Read more about Type labels in Gutenberg. Don't worry if you don't have the required permissions to add labels; the PR reviewer should be able to help with the task. |
2 similar comments
|
Warning: Type of PR label mismatch To merge this PR, it requires exactly 1 label indicating the type of PR. Other labels are optional and not being checked here.
Read more about Type labels in Gutenberg. Don't worry if you don't have the required permissions to add labels; the PR reviewer should be able to help with the task. |
|
Warning: Type of PR label mismatch To merge this PR, it requires exactly 1 label indicating the type of PR. Other labels are optional and not being checked here.
Read more about Type labels in Gutenberg. Don't worry if you don't have the required permissions to add labels; the PR reviewer should be able to help with the task. |
This was referenced Jul 23, 2024
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 does two things:
wp_theme_previewparameter.How?
@wordpress/editorpackage, so both edit-post (actions) and edit-site (quick edit) packages have access to it.@wordpress/routerpackage, so both edit-post (actions) and edit-site (quick edit) packages have access to it.Testing Instructions
Test the view action:
wp_theme_theme=twentytwentytwoURL parameter and reload. This should load TT2 in the preview.Viewaction of any page, to load the front-end.trunkthewp_theme_previewparameter is not used by the view action).Test the duplicate action: