Let editor select a featured librarian from the block editor - #224
Let editor select a featured librarian from the block editor#224djanelle-mit wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
🟡 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
expertsvia@wordpress/core-dataand 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_urlis an optional field (ACF config hasrequired: 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_urlis an optional field (ACF config hasrequired: 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.
| const { heading, featuredExpertId } = attributes; | ||
|
|
||
| const { experts, hasResolvedExperts } = useSelect( ( select ) => { | ||
| const query = { per_page: -1, status: 'publish', orderby: 'title', order: 'asc' }; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| // Look up the librarian chosen in the block editor's "Featured Librarian" panel. | ||
| $featured_expert = null; | ||
| if ( ! empty( $attributes['featuredExpertId'] ) ) { | ||
| $maybe_expert = get_post( $attributes['featuredExpertId'] ); |
| // Look up the librarian chosen in the block editor's "Featured Librarian" panel. | ||
| $featured_expert = null; | ||
| if ( ! empty( $attributes['featuredExpertId'] ) ) { | ||
| $maybe_expert = get_post( $attributes['featuredExpertId'] ); |
matt-bernhardt
left a comment
matt-bernhardt
left a comment
There was a problem hiding this comment.
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
featuredExpertIdattribute 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.
| } | ||
|
|
||
| // Fallback used when no librarian has been selected in the block editor. | ||
| $default_expert = array( |
There was a problem hiding this comment.
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:
- Defining all the
$experts...variables on lines 43-57 - Defining the alt attribute text via sprintf() on line 61
- 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)
matt-bernhardt
left a comment
matt-bernhardt
left a comment
There was a problem hiding this comment.
This looks good to me - thanks!
The CI issue is something that's going to be cleaned up in m next PR - I'm pretty sure this comes down to specifying latest as the version for dependency in package.json, rather than an actual constraint. When a package gets a new release without our having accounted for it in package-lock.json, then the CI's command to clean-install everything results in a mismatch.
The CI warns about this, if you look at the output:
`npm ci` can only install packages when your package.json and package-lock.json or npm-shrinkwrap.json are in sync. Please update your lock file with `npm install` before continuing.
The suggested fix seems like it will work, but this approach also ends up sneaking in dep updates alongside feature-oriented work, which I find unsatisfying from a "change one thing at a time" perspective.
My point in all this is: if you want to merge this as-is, the build and source materials appear to be consistent, so there'd be no problem promoting it. I'll have a PR to fix this on Monday, hopefully.
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
string incremented.
Secrets
Documentation
Accessibility
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
Dependencies
YES | NO dependencies are updated
Code Reviewer
(not just this pull request message)