Fix: New route-based admin pages are empty when no JS - #12661
Conversation
|
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 Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
There was a problem hiding this comment.
Pull request overview
This PR aims to prevent the new route-based wp-admin pages (Font Library and Connectors) from appearing blank when JavaScript is disabled by adding a user-facing description/fallback message.
Changes:
- Adds a
$descriptionstring tofont-library.phpdescribing that the screen requires JavaScript. - Adds a
$descriptionstring tooptions-connectors.phpdescribing that the screen requires JavaScript.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/wp-admin/font-library.php | Adds a new $description message intended for non-JS fallback. |
| src/wp-admin/options-connectors.php | Adds a new $description message intended for non-JS fallback. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (7)
src/wp-admin/options-connectors.php:41
- Use standard admin wrapper markup and PHP tag formatting for headings. Core uses
<div class="wrap ...">and includes semicolons/spaces inside<?php ... ?>blocks (e.g.wp-admin/site-editor.php:321-322).
<section class="wrap hide-if-js">
<h1><?php echo esc_html( $title )?></h1>
<?php
src/wp-admin/options-connectors.php:49
- Opening tag was changed from
<section>to<div>above; update the closing tag to match to avoid invalid markup.
</section>
src/wp-admin/options-connectors.php:46
wp_admin_notice()supports a dedicatedtypeargument (which generates thenotice notice-errorclasses). Passingerrorviaadditional_classesproduces a non-standard class list (e.g.notice error ...) and may not get the expected styling.
array(
'additional_classes' => array( 'error', 'hide-if-js' ),
)
src/wp-admin/font-library.php:39
- Use standard admin wrapper markup and PHP tag formatting for headings. Core uses
<div class="wrap ...">for no-JS fallbacks and includes semicolons/spaces inside<?php ... ?>blocks (seewp-admin/site-editor.php:321-322).
<section class="wrap hide-if-js">
<h1><?php echo esc_html( $title )?></h1>
<?php
src/wp-admin/font-library.php:47
- Opening tag was changed from
<section>to<div>above; update the closing tag to match to avoid invalid markup.
</section>
src/wp-admin/font-library.php:44
wp_admin_notice()supports a dedicatedtypeargument (which generates thenotice notice-errorclasses). Passingerrorviaadditional_classesproduces a non-standard class list and may not get the expected styling.
array(
'additional_classes' => array( 'error', 'hide-if-js' ),
)
src/wp-admin/css/common.css:4032
- Same as the desktop rule above:
#wpcontentalready has its own padding in the mobile breakpoint, and.wrapis styled globally. This extrapadding-inline-startlikely double-indents only these no-JS wrappers on small screens.
#wpcontent section.wrap.hide-if-js {
padding-inline-start: 10px;
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
src/wp-admin/css/common.css:17
padding-inline-startis the only use of this logical property inwp-admin/css/and may not match the rest of core admin CSS (which uses physical properties likepadding-leftand relies on RTL generation). Consider usingpadding-lefthere for consistency and broader browser compatibility.
#wpcontent section.wrap.hide-if-js {
padding-inline-start: 20px;
}
src/wp-admin/css/common.css:4037
- Same as above: using
padding-lefthere matches the surrounding mobile breakpoint rule (#wpcontent { padding-left: 10px; }) and keeps the LTR/RTL behavior consistent with the rest of core’s generated RTL stylesheets.
#wpcontent section.wrap.hide-if-js {
padding-inline-start: 10px;
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (4)
src/wp-admin/options-connectors.php:39
- This no-JS fallback wrapper will be hidden by the inline CSS emitted by
wp_options_connectors_wp_admin_render_page()(#wpbody-content > div:not(.boot-layout-container):not(#screen-meta) { display: none; }), because this<div>is a direct child of#wpbody-contentand does not have theboot-layout-containerclass. As a result, the page can still appear empty when JavaScript is disabled.
<div class="wrap hide-if-js">
src/wp-admin/font-library.php:37
- This no-JS fallback wrapper will be hidden by the inline CSS emitted by
wp_font_library_wp_admin_render_page()(#wpbody-content > div:not(.boot-layout-container):not(#screen-meta) { display: none; }), because this<div>is a direct child of#wpbody-contentand does not have theboot-layout-containerclass. As a result, the page can still appear empty when JavaScript is disabled.
<div class="wrap hide-if-js">
src/wp-admin/css/common.css:17
- This selector applies to every
div.wrap.hide-if-jsin wp-admin (e.g. block editor / site editor no-JS fallbacks), adding extra indentation even on screens that do not reset#wpcontentpadding. If the intent is to compensate for route-based pages that set#wpcontent { padding-inline-start: 0; }, consider scoping this rule to the route-based containers (e.g. requiring theboot-layout-containerclass).
#wpcontent div.wrap.hide-if-js {
padding-inline-start: 20px;
}
src/wp-admin/css/common.css:4037
- Same as above: this affects all no-JS
.wrap.hide-if-jsfallbacks in the responsive layout. Scoping it to the route-based container avoids unintended indentation changes on other admin screens.
#wpcontent div.wrap.hide-if-js {
padding-inline-start: 10px;
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (4)
src/wp-admin/options-connectors.php:39
- This no-JS fallback wrapper will be hidden by the inline “Hide legacy admin elements” CSS emitted by
wp_options_connectors_wp_admin_render_page()(#wpbody-content > div:not(.boot-layout-container):not(#screen-meta) { display: none; }), so the page can still appear empty when JavaScript is disabled. Add theboot-layout-containerclass to this wrapper so it is excluded from that hide rule.
<div class="wrap hide-if-js">
src/wp-admin/font-library.php:37
- This no-JS fallback wrapper will be hidden by the inline “Hide legacy admin elements” CSS emitted by
wp_font_library_wp_admin_render_page()(#wpbody-content > div:not(.boot-layout-container):not(#screen-meta) { display: none; }), so the page can still appear empty when JavaScript is disabled. Add theboot-layout-containerclass to this wrapper so it is excluded from that hide rule.
<div class="wrap hide-if-js">
src/wp-admin/css/common.css:17
- This selector applies padding to every no-JS wrapper that uses
div.wrap.hide-if-js(e.g. block editor, widgets editor, site editor), which broadens the impact of this change beyond the new route-based pages. Since this padding is intended to compensate for the route-based pages’boot-layout-containerlayout, scope the selector to.boot-layout-container.hide-if-jsinstead.
#wpcontent div.wrap.hide-if-js {
padding-inline-start: 20px;
}
src/wp-admin/css/common.css:4037
- This mobile breakpoint selector is currently broad enough to affect other no-JS wrappers (block editor / widgets / site editor). If the intent is to target only the route-based pages, scope it to
.boot-layout-container.hide-if-jsto match the wrapper on those pages.
#wpcontent div.wrap.hide-if-js {
padding-inline-start: 10px;
}
afercia
left a comment
There was a problem hiding this comment.
LGTM assuming the required PR for Gutenberg at WordPress/gutenberg#80628 gets merged and backported to Core. I left only two minor comments.
Thanks @afercia, I have updated the PR with the feedbacks! |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
src/wp-admin/font-library.php:37
- The no-JS fallback wrapper is likely being hidden by the inline CSS emitted by
wp_font_library_wp_admin_render_page()(#wpbody-content > div:not(.boot-layout-container)... { display:none; }). As a result, the notice still won’t be visible when JavaScript is disabled.
<div class="wrap hide-if-js">
src/wp-admin/options-connectors.php:39
- The no-JS fallback wrapper is likely being hidden by the inline CSS emitted by
wp_options_connectors_wp_admin_render_page()(#wpbody-content > div:not(.boot-layout-container)... { display:none; }). As a result, the notice still won’t be visible when JavaScript is disabled.
<div class="wrap hide-if-js">
|
I did not have enough time to include this PR in Beta4, but WordPress/gutenberg#80626 has already been shipped in Beta4. Let's aim to ship this PR in RC1. |
Trac ticket: https://core.trac.wordpress.org/ticket/65690
Use of AI Tools
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.