fix: UI review bugs#414
Conversation
871c3da to
d3b8440
Compare
|
Two actionable issues remain in this PR's exact diff:
|
|
Fixed the modal title contract and limited the manage screen to preview-only CodeMirror assets. Verified with lint, PHPUnit, build, and targeted E2E checks. |
|
One preview regression remains for users who disable syntax highlighting. |
|
Guarded the editor bootstrap so the read-only textarea remains a functional fallback when |
|
Follow-up adversarial re-audit found two remaining issues:
|
|
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. |
|
Round 4 found two issues in this PR's own head:
|
…te orphaned Prism wrapper
… drop stale manage Prism assets
There was a problem hiding this comment.
I believe this is still necessary for the front-end code highlighting?
| .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([ |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
Unnecessary in a PHP class file.
| */ | ||
| public function print_fallback_styles() { | ||
| ?> | ||
| <style> |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
This sort of processing should occur when the remote payload is being decoded, not when the property is being set.
Co-authored-by: Maisy <maisy@codesnippets.pro> Co-authored-by: Shea Bunge <shea@sheabunge.com>
…petspro/code-snippets into fix/166-ui-review-fixes/core
|
|
|
||
| .active-snippet { | ||
| td, th { | ||
| background-color: rgba(#78c8e6, 0.06); |
There was a problem hiding this comment.
use modern syntax, rgb() instead of rgba().
| background-color: rgba(#78c8e6, 0.06); | |
| background-color: rgb(120 200 230 / 0.06); |
| border: none; | ||
| background: none; | ||
| font: inherit; | ||
| color: #2271b1; |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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')} |
There was a problem hiding this comment.
Not sure how, but the string should be %1$s of %2$s.
| const TAB_LABELS: Record<CommunityTab, string> = { | ||
| snippets: __('Snippets', 'code-snippets'), | ||
| bundles: __('Bundles', 'code-snippets') | ||
| } | ||
|
|
There was a problem hiding this comment.
Don't see the advantage of this change.
| /* translators: %s: cloud library author name. */ | ||
| _x('By %s', 'snippet author', 'code-snippets'), |
There was a problem hiding this comment.
No need to use both context and translators comment.
| /* 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')} |
There was a problem hiding this comment.
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>'; |
There was a problem hiding this comment.
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>'; |
There was a problem hiding this comment.
add role=region and aria-label.
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_noticesfilter (default enabled). AddsAdmin\Notice_Filterand a unit test.Snippet preview modal
Verification
lint:js,lint:styles,lint:phpand the webpack build pass.