-
Notifications
You must be signed in to change notification settings - Fork 25
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Dashicons: Implement with Interactivity API #531
base: trunk
Are you sure you want to change the base?
Conversation
This reverts commit a04fd43.
"build": "wp-scripts build", | ||
"start": "wp-scripts start", | ||
"build": "wp-scripts build --experimental-modules", | ||
"start": "wp-scripts start --experimental-modules", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could pull this out to its own PR to upgrade the package and add this flag. I don't think it should affect anything else.
@ryelle I'd appreciate feedback on this PR, thank you! |
@@ -6,7 +6,7 @@ | |||
"license": "GPL-2.0-or-later", | |||
"private": true, | |||
"dependencies": { | |||
"@wordpress/scripts": "27.1.0" | |||
"@wordpress/scripts": "28.3.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume that the code is going to work with WordPress 6.7 because otherwise, it would fail with the missing JSX runtime script dependency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good callout!
Breaking Changes
Note If you're using @wordpress/scripts for building JS scripts to target WordPress 6.5 or earlier, you should not upgrade to this version and continue using @wordpress/scripts@27.
I think the site uses latest WordPress, so if it's on WP 6.6, it should work if that comment is accurate. Do you know if it's WordPress 6.6 or 6.7 that will use the new JSX runtime?
@ryelle can you confirm the site runs on latest WordPress release (6.6.x at this time)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, 6.6 is fine, too. I assume that it's sartisfied here. However, I wanted to ensure we don't see any surprises when deploying.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WordPress.org runs trunk & (usually) the latest Gutenberg version (the exception is the Support Forums due to conflicts with blocks-everywhere, so we used a pinned version there). So we should be set with this version change. It also worked fine on my sandbox, which matches production versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this is working just as the current page 👍🏻 I appreciate that you've kept the old URLs working, and the server-side rendering of the icon param works nicely.
There are a handful of lint errors, I know you mentioned not having it set up locally — I don't know if this helps, but this is the output at the moment (ignoring some content escaping issues that exist on the shortcode too).
$ composer lint ./source/wp-content/themes/wporg-developer-2023/src/dashicons-page
> phpcs './source/wp-content/themes/wporg-developer-2023/src/dashicons-page'
.E 2 / 2 (100%)
FILE: ./source/wp-content/themes/wporg-developer-2023/src/dashicons-page/render.php
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
FOUND 11 ERRORS AFFECTING 9 LINES
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
26 | ERROR | [x] Array keys must be surrounded by spaces unless they contain a string or an integer. (WordPress.Arrays.ArrayKeySpacingRestrictions.NoSpacesAroundArrayKeys)
27 | ERROR | [x] Array keys must be surrounded by spaces unless they contain a string or an integer. (WordPress.Arrays.ArrayKeySpacingRestrictions.NoSpacesAroundArrayKeys)
33 | ERROR | [ ] $_GET data not unslashed before sanitization. Use wp_unslash() or similar (WordPress.Security.ValidatedSanitizedInput.MissingUnslash)
33 | ERROR | [ ] Detected usage of a non-sanitized input variable: $_GET['icon'] (WordPress.Security.ValidatedSanitizedInput.InputNotSanitized)
42 | ERROR | [ ] Found single-underscore "_()" function when double-underscore expected. (WordPress.WP.I18n.SingleUnderscoreGetTextFunction)
43 | ERROR | [x] Expected 1 space after "=>"; 2 found (WordPress.WhiteSpace.OperatorSpacing.SpacingAfter)
43 | ERROR | [ ] Found single-underscore "_()" function when double-underscore expected. (WordPress.WP.I18n.SingleUnderscoreGetTextFunction)
44 | ERROR | [ ] Found single-underscore "_()" function when double-underscore expected. (WordPress.WP.I18n.SingleUnderscoreGetTextFunction)
53 | ERROR | [ ] Empty line required before block comment (Squiz.Commenting.BlockComment.NoEmptyLineBefore)
77 | ERROR | [x] Expected 1 space before comment text but found 2; use block comment if you need indentation (Squiz.Commenting.InlineComment.SpacingBefore)
158 | ERROR | [x] Opening PHP tag must be on a line by itself (Squiz.PHP.EmbeddedPhp.ContentBeforeOpen)
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 5 MARKED SNIFF VIOLATIONS AUTOMATICALLY
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
source/wp-content/themes/wporg-developer-2023/src/dashicons-page/render.php
Outdated
Show resolved
Hide resolved
source/wp-content/themes/wporg-developer-2023/src/dashicons-page/render.php
Outdated
Show resolved
Hide resolved
source/wp-content/themes/wporg-developer-2023/src/dashicons-page/render.php
Outdated
Show resolved
Hide resolved
source/wp-content/themes/wporg-developer-2023/src/dashicons-page/render.php
Outdated
Show resolved
Hide resolved
I've addressed feedback and fixed lints, this is ready for another review. |
source/wp-content/themes/wporg-developer-2023/src/dashicons-page/view.js
Outdated
Show resolved
Hide resolved
source/wp-content/themes/wporg-developer-2023/src/dashicons-page/render.php
Outdated
Show resolved
Hide resolved
Co-authored-by: Kelly Dwan <[email protected]>
Co-authored-by: Kelly Dwan <[email protected]>
Replace the dashicon page shortcode with a dashicon page block.
I had to replace the shortcode with a block because
wpautop
was running on the shortcode and mangling Interactivity API HTML.I have commented the problematic line and added a todo comment to re-enabled this when supported.
This requires WordPress/wordpress-develop#7075 to work correctly on the server. I expect that to be released in 6.6.2. To test this, you'll need to check out that PR and instruct wp-env to use that core version.I have not removed or touched the existing dashicon shortcode code for now, but there are opportunities for removing code if we go ahead with this.
Most of the page HTML is transformed from the shortcode in https://github.com/WordPress/wporg-developer/blob/trunk/source/wp-content/themes/wporg-developer-2023/inc/shortcode-dashicons.php.
Testing
Load up the page in a local environment and confirm it behaves like prodoction. Note that these pages render a random icon in detail:
"Permalinks" were rendered client site with a hash to the icon slug. This behavior has been changed to use query params so that the server can render the requested icon (hash is not available to the server). Client-side, if a fragment is found with no valid icon query param, it will be upgraded to a query param:
See the upgrade behavior, where these will behave the same (after client rewrites the URL):
Filtering should work basically the same. Try using the filter bar. Non-alphanumeric characters are ignored from filtering.
The copy buttons work the same, but they are buttons instead of
a
elements. This seems more semantic as they open a prompt.