Size Change: +87 B (0%)

Total Size: 1.84 MB

_{compressed-size-action}

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: t-hamano <wildworks@git.wordpress.org>
Co-authored-by: stokesman <presstoke@git.wordpress.org>
Co-authored-by: Mamaduka <mamaduka@git.wordpress.org>
Co-authored-by: carolinan <poena@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

However, even if a component returns null based on a certain condition, like this, the result is not equal to undefined, so the area wrapper itself will be rendered:

I haven't tested this, but can we resolve the wrapper issue by returning undefined from components instead of null? React 18 allows returning undefined from Components. See reactwg/react-18#75.

I don't have a strong opinion on Router package changes, but I'll vote for the simplest solution for this particular case.

I have tried the code below:

function HomePreview() {
	return undefined;
}

export const homeRoute = {
	name: 'home',
	path: '/',
	areas: {
		sidebar: <SidebarNavigationScreenMain />,
		preview: <HomePreview />,
		mobile: <SidebarNavigationScreenMain />,
	},
};

I was hoping that the preview area wouldn't be rendered, but at this point areas.preview is a React Element, so it renders a resizable empty canvas:

Flaky tests detected in 9bcca28.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/13544266093
📝 Reported issues:

• [Flaky Test] Redirection after template creation #60678 in /test/e2e/specs/site-editor/site-editor-url-navigation.spec.js

Thanks for the feedback!

I'm not 100% certain that this couldn’t some day be made unnecessary

Although the route registration API is private and therefore they can remove this parameter in the future if they wish, I am also concerned about this.

If we proceed more cautiously without making changes to the router package, I would like to suggest a consistent UI like the following:

• Display a warning message in the sidebar
• Display some text on empty preview or content

I don't have a strong opinion on which approach to take, but I just want to avoid UI like this (e.g. unnatural resize handles) 😅

I am less concerned about empty screens or resize handles, than about the user accessing functionality that is broken for classic theme users.

In time, isn't the plan that all of the Site Editor will work for all theme types? There will be only one theme type. So yes it should eventually be unnecessary, it will take time though.

In time, isn't the plan that all of the Site Editor will work for all theme types?

This is true, but the site editor will likely eventually be required to be made available to more than just administrators. In that case, we may want to control which screens are accessible based on user roles. This PR is also necessary to control access to stylebook based on theme support.

Either way, I think a mechanism for controlling areas based on some conditions will be necessary in the future.

If I remember correctly, the router is still private/internal API; if we find a better alternative to matchResolverArgs, it can be easily removed without worrying about backward compatibility. Is my understanding here correct?

If I remember correctly, the router is still private/internal API; if we find a better alternative to matchResolverArgs, it can be easily removed without worrying about backward compatibility. Is my understanding here correct?

Yes.

• In the @wordpress/router package, only private APIs are available.
• In the @wordpress/edit-site package, registerRoute and unregiserRoute actions are private.

Each time this component re-renders, the matchResolverArgs will be a new value, even though the currentTheme value never changes. This will invalidate memoization in useMatch, resulting in a new match.

I'm unsure what consequences this can have on the Site Editor app; it would be hard to tell without proper testing. This makes me hesitant to approve this; just days away from Beta 1.

We could try and memoize (useMemo) the matchResolverArgs value, just like beforeNavigate.

Should we freeze this PR and try the alternative approach suggested in this comment? In other words, we try a similar approach to #69005:

If we proceed more cautiously without making changes to the router package, I would like to suggest a consistent UI like the following:

• Display a warning message in the sidebar
• Display some text on empty preview or content

If navigation within the site editor is properly controlled, users should never see this screen unless they access it directly via a URL.

I think this approach can be worked on even after the beta release, because it's a "bug fix" that doesn't add any new APIs.

What do you think?

Update I added a new theme via 00766d0. This is necessary to determine whether the current theme has theme.json. See here for details.

Or, you could let enable the theme support if theme.json exists, and still:

• use the theme support check
• allow anyone to opt-out

This is necessary to determine whether the current theme has theme.json

Ideally, we would add a new field to the REST API endpoint. This is about here.

if ( rest_is_field_included( 'has_theme_json', $fields ) ) {
	$data['has_theme_json'] = wp_theme_has_theme_json();
}

However, I am hesitant to add such a new field just before the Beta release.

@t-hamano, are there any blockers here? Can we merge this?

Don't forget to add the "Backport to beta" label before merging.

@Mamaduka After this PR was approved, I added one more selector by 00766d0.

This selector is needed to determine if the theme has a theme.json file. See #68036 (comment) for details.

I just cherry-picked this PR to the wp/6.8 branch to get it included in the next release: 335079a