Skip to content

fix: UI review bugs#414

Open
imantsk wants to merge 36 commits into
core-betafrom
fix/166-ui-review-fixes
Open

fix: UI review bugs#414
imantsk wants to merge 36 commits into
core-betafrom
fix/166-ui-review-fixes

Conversation

@imantsk

@imantsk imantsk commented Jul 13, 2026

Copy link
Copy Markdown
Member

Changes

Cloud snippet descriptions render as plain text

Community Cloud snippet descriptions are stripped of HTML tags before display, in both the card and table views.

Foreign admin notices hidden on plugin screens

On Code Snippets admin screens, admin notices that do not originate from the plugin are removed server-side and hidden via a CSS fallback, so core/third-party notices no longer break the plugin layout. Behaviour is gated by the code_snippets/admin/filter_foreign_notices filter (default enabled). Adds Admin\Notice_Filter and a unit test.

Snippet preview modal

  • Minimum and maximum dimensions with internal scrolling for small and large snippets.
  • Renders a read-only editor that honours the configured editor theme.

Verification

  • lint:js, lint:styles, lint:php and the webpack build pass.
  • Notice filtering, modal sizing, and preview theming confirmed in wp-admin.

@imantsk
imantsk force-pushed the fix/166-ui-review-fixes branch from 871c3da to d3b8440 Compare July 14, 2026 10:15
@imantsk imantsk changed the title fix: UI review bugs (#166) — core fix: UI review bugs (#166) Jul 14, 2026
@imantsk
imantsk marked this pull request as ready for review July 14, 2026 12:52
@code-snippets-bot

code-snippets-bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Two actionable issues remain in this PR's exact diff:

  • High — TypeScript regression in the preview modal. src/js/components/common/SnippetPreviewModal.tsx:61-66 passes a JSX <span> to the WordPress Modal title prop, which is declared as title?: string. tsc --noEmit reports TS2322: Type 'Element' is not assignable to type 'string'. Keep title as the snippet-name string and render the badge separately, or provide an accessible custom header without weakening the type.
  • Medium — Avoid loading the full editable-editor stack for a read-only preview. src/php/Admin/Menus/Manage_Menu.php:222 calls enqueue_code_editor('php'), which also enqueues HTMLHint, CSSLint, JSHint, and the plugin's editable-editor bundle. These load on every Manage screen even when the preview never opens. Enqueue only the core CodeMirror assets/settings and selected theme required by the read-only modal.

@imantsk

imantsk commented Jul 17, 2026

Copy link
Copy Markdown
Member Author

Fixed the modal title contract and limited the manage screen to preview-only CodeMirror assets. Verified with lint, PHPUnit, build, and targeted E2E checks.

@code-snippets-bot

Copy link
Copy Markdown
Contributor

One preview regression remains for users who disable syntax highlighting. enqueue_code_preview_editor() relies on wp_enqueue_code_editor() (src/php/Utils/editor.php:115-123), but WordPress returns false and does not enqueue code-editor when the current user's syntax_highlighting preference is disabled. SnippetPreviewModal.tsx:43 then unconditionally calls window.wp.codeEditor.initialize(...), so opening Preview throws instead of displaying the code. Please guard initialization and retain the read-only textarea as a functional fallback when wp.codeEditor is unavailable, and add coverage for the disabled-preference path.

@imantsk

imantsk commented Jul 17, 2026

Copy link
Copy Markdown
Member Author

Guarded the editor bootstrap so the read-only textarea remains a functional fallback when wp.codeEditor is unavailable, and added E2E coverage for the disabled syntax-highlighting preference.

@code-snippets-bot

Copy link
Copy Markdown
Contributor

Follow-up adversarial re-audit found two remaining issues:

  1. The fallback E2E test depends on a view it does not establish. tests/e2e/code-snippets-preview.spec.ts:43-49 uses the helper and selector for .wp-list-table, but this PR still defaults an unset snippet-view preference to card in Preferences_REST_Controller.php:42. On a clean test user, the test can time out before opening Preview; Playwright is also skipped in the current checks. Please explicitly set the preference to table in setup, or exercise the card UI, so the fallback path is actually covered.

  2. The notice ownership check accepts similarly prefixed sibling directories. Notice_Filter.php:155 uses 0 === strpos( $file, dirname( PLUGIN_FILE ) ). A callback file under a sibling such as code-snippets-extra/ therefore passes as plugin-owned and its notice survives filtering. Please compare normalized paths with a trailing directory-separator boundary and add a sibling-prefix regression case.

@imantsk

imantsk commented Jul 18, 2026

Copy link
Copy Markdown
Member Author

Resolved both follow-ups. The fallback E2E setup now explicitly establishes table view before using table selectors, and notice ownership compares normalized plugin-root paths with a directory-separator boundary, including a sibling-prefix regression case. The relevant tests, lint, and build pass.

@code-snippets-bot

Copy link
Copy Markdown
Contributor

Round 4 found two issues in this PR's own head:

  1. Notice_Filter::is_code_snippets_screen() checks only each menu's primary hookname (src/php/Admin/Notice_Filter.php:114-121). The editor's separate Add New hook is therefore not recognized, so foreign notices remain visible on that Code Snippets screen. Please include every hook registered by a menu and add behavioral coverage for Add New. The later foundation branch already contains this repair, so the smallest fix is to bring that implementation and test back here.

  2. SnippetPreviewModal.tsx uses CodeMirror's readOnly: 'nocursor'. In CodeMirror 5 that mode disallows focusing the editor, which prevents keyboard users from reaching and selecting the preview text. Use readOnly: true instead; the later preview-actions branch already contains the canonical fix and keyboard coverage.

@code-snippets-bot code-snippets-bot changed the title fix: UI review bugs (#166) fix: UI review bugs Jul 18, 2026
Comment thread src/js/utils/text.ts Fixed
@code-snippets-bot code-snippets-bot added the run-tests Trigger automated tests label Jul 19, 2026
@code-snippets-bot code-snippets-bot added build Adding this label will trigger the zip build action and removed build Adding this label will trigger the zip build action run-tests Trigger automated tests labels Jul 19, 2026
Comment thread src/js/utils/Prism.ts Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is still necessary for the front-end code highlighting?

Comment thread src/js/utils/text.ts
.replace(/<\/?[a-z][a-z0-9]*\b[^>]*>/gi, '')
// Tags implying a visual break become whitespace so `<p>A</p><p>B</p>`
// yields 'A B' rather than 'AB'; inline tags are removed without separators.
const BLOCK_TAG_NAMES = new Set([

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems unnecessarily complex for what it is trying to do. I'm not confident that we can be clear that it is doing its job, especially when it is serving an important security role.

use function Code_Snippets\code_snippets;
use const Code_Snippets\PLUGIN_FILE;

defined( 'ABSPATH' ) || exit;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary in a PHP class file.

*/
public function print_fallback_styles() {
?>
<style>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to do all of this reflection processing when we can just hide the notices with CSS?

return (bool) $value;
case 'description':
return ( null === $value ) ? '' : $value;
// Remote payloads occasionally carry non-string descriptions;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sort of processing should occur when the remote payload is being decoded, not when the property is being set.

imantsk and others added 3 commits July 21, 2026 15:12
@sheabunge sheabunge self-assigned this Jul 21, 2026
@code-snippets-bot

Copy link
Copy Markdown
Contributor
  • [High] Preserve existing local snippet descriptions — src/php/Model/Snippet.php:134

  • prepare_field() now runs every description through wp_kses_post(), while the same model hydrates existing local database rows. Opening and then saving a legacy snippet can permanently strip previously stored markup outside the post allowlist. Keep generic local-model hydration non-destructive; sanitize cloud descriptions at the cloud import/download boundary or escape at rendering, and add a legacy-description round-trip regression test.

  • [Medium] Use the cloud page-size default for featured results — src/php/REST_API/Cloud/Cloud_Snippets_REST_Controller.php:248

  • After the schema default was removed, each endpoint callback owns its fallback. Search uses Manage_Menu::get_cloud_search_per_page(), but featured results use get_snippets_per_page(), so the initial featured page ignores the documented code_snippets/cloud_search/per_page filter and can return a different count from searches. Use the cloud-search default here and cover the featured endpoint default/filter behavior.

  • [Medium] Restore per-row selection in cloud table view — src/js/manage/cloud/CloudSnippetsTable.tsx:20

  • The cloud table is now the default view and its toolbar exposes bulk Download plus Select all, but table rows render no selection control; only cards receive the per-item selection handler. Table users therefore cannot download an arbitrary subset. Pass the selection state/handler into the table and render row and header checkboxes, or remove bulk selection from table view.


.active-snippet {
td, th {
background-color: rgba(#78c8e6, 0.06);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use modern syntax, rgb() instead of rgba().

Suggested change
background-color: rgba(#78c8e6, 0.06);
background-color: rgb(120 200 230 / 0.06);

border: none;
background: none;
font: inherit;
color: #2271b1;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All #2271b1 colors can be replaced with sass theme.$accent. there are multiple instances that can be changed.

inline-size: 20px;
block-size: 20px;
margin: 0;
border: 1px solid #c3c4c7;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

replace #c3c4c7 with theme.$control-border. Do that in other places too where the corder color is hardcoded.

ul.cloud-search-results.code-snippets-cards {
grid-template-columns: repeat(3, minmax(0, 1fr));

@media screen and (width <= 1100px) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove screen and from the media query.

<PagingInput which={which} totalPages={totalPages} {...inputProps} />
<span className="tablenav-paging-text">
{/* translators: 1: Current page. */
_x(' of ', 'paging', 'code-snippets')}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure how, but the string should be %1$s of %2$s.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and use sprintf).

Comment on lines +16 to +20
const TAB_LABELS: Record<CommunityTab, string> = {
snippets: __('Snippets', 'code-snippets'),
bundles: __('Bundles', 'code-snippets')
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't see the advantage of this change.

Comment on lines +28 to +29
/* translators: %s: cloud library author name. */
_x('By %s', 'snippet author', 'code-snippets'),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to use both context and translators comment.

Suggested change
/* translators: %s: cloud library author name. */
_x('By %s', 'snippet author', 'code-snippets'),
/* translators: %s: cloud library author name. */
__('By %s', 'code-snippets'),

when using context function _x() it creates a separate string, inflating the amount of strings to translate.

id="snippets-tag-filter"
name="tag"
value={currentTag}
aria-label={__('Filter snippets by tag', 'code-snippets')}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aria-label is not required, it has a <label> pointing to this <select> element.

echo '<div class="error fade"><p><strong>';
// The code-snippets-notice class exempts this notice from the plugin's
// own admin notice filtering on Code Snippets screens.
echo '<div class="code-snippets-notice error fade"><p><strong>';

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add role=region and aria-label.


if ( defined( 'CODE_SNIPPETS_FILE' ) ) {
echo '<div class="error fade"><p>';
echo '<div class="code-snippets-notice error fade"><p>';

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add role=region and aria-label.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants