Let editor select a featured librarian from the block editor

[djanelle-mit]

Developer

To avoid manual weekly updates, this work introduces a dropdown in the block editor side panel to allow the editor to change the featured librarian.

This pulls from the existing Experts custom post type.

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

[Copilot]

🟡 Changes recommended

The editor query uses an invalid per_page value and the server render path needs small hardening to avoid empty expert links and ensure safer ID handling.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds editor-configurable selection of a “Featured Librarian” for the Featured & Events block by sourcing options from the existing experts custom post type and rendering the selected expert’s details on the front end (with a default fallback).

Changes:

• Adds a sidebar (“Inspector”) dropdown that loads published experts via @wordpress/core-data and stores the selected expert ID in block attributes.
• Updates server-side rendering to use the chosen expert’s title, excerpt, thumbnail, and URL (with a hard-coded default when none is selected).
• Updates generated build artifacts (JS, asset deps, block metadata, and manifest) to include the new attribute and dependencies.

File summaries

File	Description
web/app/plugins/mitlib-blocks/package.json	Adds WP data packages needed for core-data selection in the editor.
web/app/plugins/mitlib-blocks/build/featured-and-events-section/render.php	Built SSR template now renders selected expert details.
web/app/plugins/mitlib-blocks/build/featured-and-events-section/index.js	Built editor code adds InspectorControls dropdown backed by core-data.
web/app/plugins/mitlib-blocks/build/featured-and-events-section/index.asset.php	Updates script dependencies to include wp-data/wp-core-data/wp-components.
web/app/plugins/mitlib-blocks/build/featured-and-events-section/block.json	Built block metadata includes featuredExpertId attribute.
web/app/plugins/mitlib-blocks/build/blocks-manifest.php	Built manifest includes featuredExpertId attribute for registration.
web/app/plugins/mitlib-blocks/blocks/featured-and-events-section/render.php	Source SSR template now renders selected expert details.
web/app/plugins/mitlib-blocks/blocks/featured-and-events-section/edit.js	Source editor UI adds InspectorControls dropdown backed by core-data.
web/app/plugins/mitlib-blocks/blocks/featured-and-events-section/block.json	Source block metadata includes featuredExpertId attribute.

Review details

Files not reviewed (1)

• web/app/plugins/mitlib-blocks/build/featured-and-events-section/index.js: Generated file

Suppressed comments (2)

web/app/plugins/mitlib-blocks/blocks/featured-and-events-section/render.php:46

• expert_url is an optional field (ACF config has required: 0), so this may be empty; rendering <a href=""> will link back to the current page. Provide a fallback URL (e.g., the expert post permalink) when the meta value is missing.

					$expert_url        = get_post_meta( $featured_expert->ID, 'expert_url', true );

web/app/plugins/mitlib-blocks/build/featured-and-events-section/render.php:46

• expert_url is an optional field (ACF config has required: 0), so this may be empty; rendering <a href=""> will link back to the current page. Provide a fallback URL (e.g., the expert post permalink) when the meta value is missing.

					$expert_url        = get_post_meta( $featured_expert->ID, 'expert_url', true );

• Files reviewed: 8/9 changed files
• Comments generated: 3
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[djanelle-mit]

Asked VS Code's copilot about this feedback:

Why per_page: -1 won't fail here: @wordpress/api-fetch ships a fetchAllMiddleware that's part of apiFetch's default middleware stack (used by @wordpress/core-data). When it sees per_page: -1 in the query, it never actually sends -1 to the REST endpoint — it strips it, requests page 1 with a large bounded per_page, then keeps requesting subsequent pages and concatenating results until it runs out, based on the X-WP-Total/X-WP-TotalPages headers. This is the same mechanism core Gutenberg blocks rely on (e.g. the Categories block, Page List block, Query Loop's post-type dropdowns) to fetch "all" entities regardless of count. So the dropdown won't come back empty because of this — it works by design.

[djanelle-mit]

Leaving this alone for the time being. I don't expect this field to ever have more than 99 experts, but if this is common convention I'd rather go down that route.

[djanelle-mit]

Updated in both places.

[matt-bernhardt]

Okay, this took a bit longer to write up and explore than I originally anticipated - so I appreciate your patience.

To start, this works and does so in a pretty elegant way. I like that we can define a lookup field as an attribute as efficiently as we are between block.json and edit.js - this is really encouraging as we move forward into the block regime.

My feedback largely centers on a separation of concerns in render.php, wanting to keep variable assignment separate from the details of the markup structure. There are four comments in that template, and a suggestion for what I think is a way forward. If this suggestion makes sense to you, I'd put this forward as a requested change - but I'm open to a contrary perspective here.

Beyond that request, my feedback is more tentative:

• There are two places where I'm asking questions, one about the mixing of terminology for "librarians" and "experts" because that's been a point of Discourse in the past - and I have a preference for consistency.
• I think the handling of the featuredExpertId attribute could be handled more efficiently - but what you've written does work, so I'm not requiring a change.
• There's also an aside about the use of translation functions around our codebase generally, which is something I'd like to talk about at some point - but I don't see myself ever requiring an actual change around its use.

[matt-bernhardt]

Question:

I'm a little concerned about mixing the terminology of "experts" and "librarians" for this feature.

There may not be an issue with referring to this current set of colleagues as librarians, but my recollection is that we went with "experts" for the post type name in order to avoid any concerns over who is or isn't appropriate to use that title with.

[djanelle-mit]

Makes sense... I'll update to Experts

[matt-bernhardt]

Could we calculate the alt attribute text alongside all the other expert values, rather than amongst the markup like this? I worry about losing the legibility of both the value's calculation and the logic of the markup when we intermix them like this.

[matt-bernhardt]

Same comment here as about the alt attribute text, about doing the value calculation separately from the markup.

Additionally, it looks like this is the only place where we're using $expert_first_name, so maybe what we need is just one value of How can first_name help you? that's calculated up where everything else is?

[matt-bernhardt]

I'm more okay with this block being where it is, but when I read the template it doesn't seem obvious to me that we'd perform this operation here in the markup, versus up above after line 27.

Strictly speaking, I suspect we could combine lines 21-27 and lines 43-57 into something like (in pseudocode)

// Define all to-be-rendered values using a default record
$expert_name = 'Alejandro Paz';
$expert_question = 'How can Alejandro help you?';
...
// If we've loaded a valid expert, replace those to-be-rendered values with something real
if ( $featured_expert ) {
  $expert_name = get_the_title( $featured_expert );
  $expert_question = sprintf( 'How can %s help you?', strtok( $expert_name, ' ' ) );
  ...
}

This would allow us to start the markup block below with everything already prepared, leaving only the need for the escaping functions and conditional presence checks within the markup - which allows future-us to focus more on both the markup logic and the value assignments where they're the only thing happening.

[djanelle-mit]

This makes sense... even if it's in the same file help separate the data from the presentation a bit more. Updating.

[matt-bernhardt]

Non-blocking comment:

This works, so please keep it if you'd prefer this approach. That said, I think there's a cleaner way to express this which avoids the complexity of a ternary and the empty() check:

$featured_expert_id = absint( $attributes['featuredExpertId'] ?? 0 );

Under the hood, absint() is using PHP's built-in type casting expression (int), which is extremely permissive and returns a number in all cases - even when it receives a NULL (which is the primary concern here, for the moment when we first populate this setting). Just using absint( $attributes['featuredExpertId'] ) will always return 0, although it will throw warnings (not errors) if PHP tries to evalute $attributes['featuredExpertId'] and either can't find the array at all or the key isn't yet defined. This is where the ?? 0 coalesce operator comes in, because PHP just quietly falls back to 0 without the warnings.

(I'm learning about coalesce operators now, but they were apparently introduced in PHP 7 - TIL...

[matt-bernhardt]

probably a requested change?

I think it will be cleaner and more supportable if we handle all the variable assignments up here, prior to the start of markup beginning on line 28. There are three operations happening within the markup that I fear are going to make supporting this harder over time:

1. Defining all the $experts... variables on lines 43-57
2. Defining the alt attribute text via sprintf() on line 61
3. Defining the "How can $NAME help you?" question via sprintf() on lines 73-77

(The block to compile the events down on lines 131-217 is another concern, but I'm setting that aside since it's already merged)

I think I'd propose that we handle all the variable assignment in one chunk, and leave the markup portions of this template to only call the escaping functions.

This said, if you feel strongly that we need to merge this and just be done with it, I'm open to pushback here - this seems to work.

(As an aside - In my suggested code below, I'm also dropping the use of the __() translation function. This is intentional, but I'm not super committed to not using it here. We've never made any real move to support translation, so I've been Occam's razoring it out over time, but I'm also happy to hear about a push to move in that direction in a more systematic way)