LNHU-225: Custom post type and editor UI dropdown for Hero Images by djanelle-mit · Pull Request #232 · MITLibraries/mitlib-wp-network

djanelle-mit · 2026-09-24T19:44:46Z

Developer

This work adds a plugin that introduces a new custom post type "Hero Images". This plugin also contains the homepage hero image block, which now allows the editor to select a hero image from the CPT via a dropdown in the block editor UI.

This is the first instance of our architectural decision to colocate the definitions of new data objects with all the instances of where they're rendered.

Stylesheets

Any theme or plugin whose stylesheets have changed has had its version
string incremented.

Secrets

All new secrets have been added to Pantheon tiers
Relevant secrets have been updated in Github Actions
All new secrets documented in README

Documentation

Project documentation has been updated
No documentation changes are needed

Accessibility

ANDI or Wave has been run in accordance to
our guide and
all issues introduced by these changes have been resolved or opened as new
issues (link to those issues in the Pull Request details above)

Stakeholder approval

Stakeholder approval has been confirmed
Stakeholder approval is not needed

Dependencies

YES | NO dependencies are updated

Code Reviewer

The commit message is clear and follows our guidelines
(not just this pull request message)
The changes have been verified
The documentation has been updated or is unnecessary
New dependencies are appropriate or there were no changes

matt-bernhardt

I'm flagging two changes (one of which I'm probably happy to let go if it's being too strict). The rest are non-blocking comments about some details that I see here and there.

Overall, I like where this gets us - being able to manage these images via the CMS instead of via PRs is a significant improvement on both of our workloads.

matt-bernhardt · 2026-09-30T20:05:11Z

+ * @see https://make.wordpress.org/core/2025/03/13/more-efficient-block-type-registration-in-6-8/
+ * @see https://make.wordpress.org/core/2024/10/17/new-block-type-registration-apis-to-improve-performance-in-wordpress-6-7/
+ */
+function register_blocks() {


Non-blocking comment:

I'm a little uneasy about conflating hooks to classes and hooks to functions like this, but not so much that I'm going to ask for a change now. It may never be an issue, in which case no harm done.

It wouldn't be difficult to swap this to a class method if it does turn out to be problematic.

This is a place where Copilot did the scaffolding, and I definitely need to understand this distinction better.

Do you have an example or pseudo code for the distinction between the two options?

There's a proof of concept for the difference at #236. This isn't something I expect to merge, but it shows what the alternative implementation would be. This function itself is pretty simple, and doesn't pollute the root PHP script terribly - so I'm happy to leave it for now. If we end up adding more complexity to this plugin, however, this might become more warranted.

matt-bernhardt · 2026-09-30T20:15:43Z

+			'hero_images',
+			'meta',
+			array(
+				'get_callback'    => function( $data, $field, $request, $type ) {


Maybe a requested change?

PHPCS is flagging the need for a space between function and the parentheses on this line. If you'd rather not deal with this now, I'm happy to leave it be - but I'm hoping to tackle a maintenance ticket for our plugins soon-ish that would pick it up at that point.

Would the maintenance ticket for the plugins apply this across the board (vs making a tactical fix only for this instance)? I think I'll leave this as-is for now.

The scope of the maintenance ticket is a bit nebulous - I've just been noticing that there's a small amount of technical debt like this in otherwise-clean plugins, and I've found it much easier to "hold the line" if we're always starting from a clean slate, rather than "there's a small number of things we're ignoring". It's a lot more noticeable when previously empty output now has something, than when a list of two items becomes a list of three.

matt-bernhardt · 2026-09-30T20:18:51Z

+}
+
+// Require the necessary classes.
+require_once( plugin_dir_path( __FILE__ ) . 'src/class-heroimage.php' );


Non-blocking comment:

require_once does not need parentheses (it is a statement, not a function). What you've got here is consistent with all other plugins, so I like that we're being consistent here. I'm mentioning it now so that there's background when I set up a PR to change all of these plugins at the same time as part of a maintenance ticket.

matt-bernhardt · 2026-09-30T20:23:19Z

+            "label": "Citation",
+            "name": "citation",
+            "type": "text",
+            "instructions": "The text used to credit the source of the hero image, e.g. \"Muriel Cooper personal archives\".",


Requested change:

Could we provide an example that uses different content for this citation field than what's used in the citation link text field? Setting this instruction to something like from the Muriel Cooper personal archives would make the distinction between the two fields a bit clearer, IMO.

Good call! Updated.

matt-bernhardt · 2026-09-30T20:43:01Z

+			<?php if ( $hero_credit ) : ?>
+			<span class="hero-image-credit">
+				<?php
+				// phpcs:disable WordPress.Security.EscapeOutput.OutputNotEscaped -- built from esc_html()/esc_url() pieces above.


Non-blocking comments:

It looks like you've been using PHPCS locally, if you're selectively turning off checks like this? How has that process been so far?

If I follow the logic of the variable names used on lines 47-57, I wonder if the final output should be $escaped_hero_credit to reflect that the contents have already been processed via an escaping function?

I think I like the approach of relying on context-specific escaping functions while compiling the value - if we wanted to be particularly robust while still enforcing escaping at this level, we could use wp_kses to strip out all markup except for the link with the expected attributes here, and satisfy the requirements of this sniff.

I've been using CoPilot to help move things around or scaffold things, and PHPCS and the npm run build are always run as part of those. It's been helping my commits be more consistent, so it's easier to see when something is actually changed versus me forgetting to format something correctly.

Yes, I think being as crisp as possible with variables names makes a ton of sense. Updated.

I'm not sure I follow what this means

There's a proof of concept for using wp_kses at #237 - as with the other alternate, this isn't something I expect to be merged - just a demonstration for how the pattern works. We wouldn't necessarily drop the compilation step from lines 47-57, all those escaping and protection steps are likely still necessary (certainly the esc_url and esc_attr methods are doing work that wp_kses doesn't) - but it would allow us not to turn off the protection of the sniff at the rendering line, while still enforcing an allowlist of only the markup we expect to be present.

matt-bernhardt

I see the updated instructions values, which is the one thing I felt most strongly about. I've set up demonstrations for the two code patterns you asked about, but left them as draft because I don't expect them to merge - they're for discussion purposes only.

djanelle-mit marked this pull request as draft September 24, 2026 19:44

djanelle-mit marked this pull request as ready for review September 29, 2026 18:28

djanelle-mit force-pushed the lnhu-225 branch from 3903fe0 to 780bf96 Compare September 29, 2026 18:58

djanelle-mit requested a review from matt-bernhardt September 29, 2026 19:01

matt-bernhardt requested changes Sep 30, 2026

View reviewed changes

djanelle-mit added 7 commits October 1, 2026 10:16

First pass of new Hero Image custom post type

e6a3ec7

Moved hero image plugin from mitlib-blocks with no other changes

f3350a4

Fixing merge conflict

59940ec

Ensure link text ignores leading and trailing whitespace

5fd7889

Updated to use Featured Image alt text directly from media library

ff17e8b

Updated instructions for citation field on hero image

e50e865

Renamed citation text output variable

22b1762

djanelle-mit force-pushed the lnhu-225 branch from 780bf96 to 22b1762 Compare October 1, 2026 14:50

djanelle-mit requested a review from matt-bernhardt October 1, 2026 14:52

matt-bernhardt approved these changes Oct 2, 2026

View reviewed changes

Conversation

djanelle-mit commented Sep 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Developer

Stylesheets

Secrets

Documentation

Accessibility

Stakeholder approval

Dependencies

Code Reviewer

Uh oh!

matt-bernhardt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matt-bernhardt left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

djanelle-mit commented Sep 24, 2026 •

edited

Loading