Proposal: Add On This Day Widget#11630
Conversation
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @escapemanuele. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Nice, nice! I must have some CSS issue here as I do not see the dates on the left. |
It looks like the Playground is running an outdated build. I rebased to rebuilt. Hopefully that will fix it. Edit: It's a playground issue, I updated the testing steps. |
|
Tested locally and it loads just fine! |
|
Redesigned following this. 
|
jeherve
left a comment
jeherve
left a comment
There was a problem hiding this comment.
I've been poking at the query logic and comparing against my own little plugin. I thought I'd mention a few patterns I picked up from feedback from users.
Use date_query instead of raw SQL
Right now filter_posts_where() uses a posts_where filter with hand-rolled MONTH()/DAY()/YEAR() comparisons. It works, but WP_Query already supports this natively through date_query; it handles the escaping, uses the documented API surface, and is friendlier to anyone who later wants to extend the behavior. class-query.php from the plugin is a reasonable reference.
Consider widening the window beyond the exact day
Of note, most "memory" products — Google Photos, Apple's On This Day, Facebook Memories — don't restrict to the exact calendar day; they widen the window to nearby dates so something shows up even on slow days. The plugin started the same way this PR does, and I eventually moved to a week-long window by default, with exact-day matching as an opt-in.
A couple of reasons this matters:
- Feb 29. With exact matching, leap-day posts only surface once every four years, and on Feb 29 in a non-leap year… well, that day doesn't exist, so the widget is awkwardly blank.
- Sparse posters. Someone who publishes weekly but not daily will see the empty state on most days of the year; a small window (±3 days?) turns that into a useful recap instead.
I'm wondering if we could default to a modest window and leave exact-match behind a filter for folks who really want it. Closer to how the genre works in the wild.
<time datetime="…"> needs a timezone
Small thing on the post meta row:
$time_iso = get_the_time( 'Y-m-d H:i', $post );
// ...
<time datetime="<?php echo esc_attr( $time_iso ); ?>">get_the_time() returns the post time in the site's timezone, but the emitted string has no offset or Z. Per the HTML spec that's a "local date and time" without context, so screen readers and timezone-aware tooling interpret it as the user's local time rather than the site's. Either drop to Y-m-d (a plain date is a valid datetime value), or switch to get_the_time( 'c', $post ), which gives you ISO 8601 with an offset attached.
| * | ||
| * @since 7.1.0 | ||
| */ | ||
| #[AllowDynamicProperties] |
There was a problem hiding this comment.
Why is allow dynamic properties needed? As it is new code, I would refrain from adding this.
There was a problem hiding this comment.
Great point. I was undecided about this attribute tbqh. But I was mimicking Site Health and decided to keep it for consistency. But it's not needed for my class.
I removed it.
|
Thanks for the amazing feedback, @jeherve!
Done.
I added a minimal slider to keep the noise down the allows adjusting the range from 1 to 7 days.
Fixed. |
dmsnell
left a comment
dmsnell
left a comment
There was a problem hiding this comment.
all of the calls to translation which directly output (e.g. via printf()) need to be escaped so they don’t break the page. this includes calls to _e().
happy to give another round here. looks like a nice widget
| 'on-this-day', | ||
| sprintf( | ||
| '#dashboard_on_this_day{--otd-today:%s;}', | ||
| wp_json_encode( self::get_window_label( self::get_window_days() ) ) |
There was a problem hiding this comment.
not sure what the intention here is with JSON encoding, but this is CSS, which does not understand JSON.
if you are looking to escape a CSS string it would be best to follow CSS language rules otherwise this will open up unexpected corruption.
calling @sirreal on this one, but I would imagine that this would be preferable to json_encode()
$escaped_label = strtr( self::get_window_label( ... ), '"', '\"' );there are other details, like forbidding newline characters or invalid UTF-8 in the string, but JSON encoding has its own list of corrupting circumstances
There was a problem hiding this comment.
This is a common repurposing of json_encode to wrap loose strings with quotes and escape any occurring quotes if any. It also escapes new lines and a few other baddies. Frankly I think it's probably safer than making my own function. But GPT-5.5 obliged and created a helper.
Happy to settle for either.
There was a problem hiding this comment.
I hear you, and I’ll defer to @sirreal who can speak much more eloquently on this than I can.
having a wrapper at least leaves more intention in the code which can be later cleaned up, but using JSON serialization hides that intention. there is work to add string escaping in Core, which will be preferable here anyway once it arrives.
There was a problem hiding this comment.
OK I side stepped this by avoiding CSS props in favor of HTML attributes.
alshakero
force-pushed
the
add/on-this-day-widget
branch
from
2664880 to
b4c40ed
Compare
It might be tricky to show nothing at all. Because the widgets are customizable by the user and dynamically hiding them may mess up the ordering. This is what Years Ago Today shows. 
Happy to adopt that, the UI is simpler and more performant (no carousel CSS and JS). But I'm curious which part would you like me to bring over?
|
A new, separate widget would not be very valuable on a site with zero published posts, and it would be inappropriate for sites that never have any published posts. The existing Activity widget has a 'No activity yet!' fallback when there are no future or published posts and no recent comments. I think the Published On This Day list could fit inside that widget, possibly with the same styling and similar links. A made a quick proof of concept on my fork. |
peterwilsoncc
left a comment
peterwilsoncc
left a comment
There was a problem hiding this comment.
I've added a few more notes inline, thanks for following up my earlier review.
| endif; | ||
| ?> | ||
| <ul class="wp-on-this-day-timeline<?php echo $is_carousel ? ' wp-on-this-day-is-carousel' : ''; ?>"> | ||
| <?php foreach ( $posts as $index => $post ) : ?> |
| } | ||
|
|
||
| $chunk = $processor->get_modifiable_text(); | ||
| $parts[] = $chunk; |
There was a problem hiding this comment.
I agree with Dennis that this would be clearer with concatenation.
| $clauses = array(); | ||
|
|
||
| for ( $offset = -self::WINDOW_BEFORE_DAYS; $offset <= self::WINDOW_AFTER_DAYS; $offset++ ) { | ||
| $day_date = $date->modify( ( $offset >= 0 ? '+' : '' ) . $offset . ' days' ); |
There was a problem hiding this comment.
In a comment @jeherve use leap days as an example that needs to be considered. As written this code will only display leap days in a leap year as the offset is based on the current year, ie in 2026 the combination 2, 29 won't be included.
Maybe you can add it in for the combo 2,28 if $date->format( 'L' ) == 0 but Jeremy might have a better solution.
Yes. Apparently the grid reflows nicely when an item is removed. We now don’t render anything for empty days.
Yes, I took a lot of inspiration from that widget. Here’s how it’s looking now: 
I have two questions:
@peterwilsoncc thank you so much for your generous feedback. I addressed all the parts that apply to the new design. |
dmsnell
left a comment
dmsnell
left a comment
There was a problem hiding this comment.
@alshakero thanks for your patience with my review, as I was delayed in getting back to this.
I left some comments in there, but what else do you think this needs before it’s ready? is there any help you want or think is necessary?
| * @param int $user_id Current user ID for filter context. | ||
| * @return WP_Post[] Array of posts ordered by newest first. | ||
| */ | ||
| public static function get_posts( $user_id ) { |
There was a problem hiding this comment.
just a little thought: if this shows up in diagnostics or profiling data, it should have the class prepending in its name, but also calling it something like get_posts_on_this_day() would make that even clearer at a glance.
| * | ||
| * Note: I made the trade-off to ignore `date_format` and `time_format` | ||
| * option changes. They do not bust the cache; stale date strings clear | ||
| * on the next post mutation or at midnight. |
There was a problem hiding this comment.
Nitpick: we are going to need to rewrite this to remove the first-person dialog.
/**
*
* Note: Ignoring `date_format` and `time_format` option changes is intentional.
* This is because stale date strings clear on the next post
* mutation or next midnight hour.though I think "clear on the next midnight" is a bit unclear to me. I’m not sure what all it means, so perhaps it could use some rewording, or perhaps I am just not familiar-enough with this to know.
| * Builds the date query clause for today's anniversary date. | ||
| * | ||
| * On February 28 in a non-leap year, February 29 posts are included so | ||
| * leap-day anniversaries still appear. |
There was a problem hiding this comment.
is this not backwards? since every year contains a February 28, but only every 4 years (less every hundred except every four hundred) contain a February 29?
I would expect that on Feb 28 we always see Feb 28 posts, but on Feb 29 we also include Feb 28
There was a problem hiding this comment.
I'm not sure. I think we have different product intentions. Without any special clauses, if you posted a post on Feb 29th, you won't be reminded about it for 4 years, but we want you to be.
So on 28th Feb of a non-leap year, we want to remind you about posts from Feb 29th because the next day wouldn't be one and you'd have to wait too long to see your posts from the last leap year.
There was a problem hiding this comment.
The logic here makes sense to me, per @alshakero's response.
| <ul class="wp-on-this-day-years"> | ||
| <?php foreach ( $posts_by_year as $year => $year_posts ) : ?> | ||
| <li class="wp-on-this-day-year"> | ||
| <h3 class="wp-on-this-day-year-heading"><?php echo esc_html( $year ); ?></h3> |
There was a problem hiding this comment.
any reason to add data-post-year on this? or some attribute or class name which would be useful to stylers?
There was a problem hiding this comment.
Sorry, I don't understand. Would you like me to add an attribute?
There was a problem hiding this comment.
I think @dmsnell is just asking whether it might make sense to add an attribute that extenders can use for styling.
I'm not sure I can see a lot of reason for that; obviously people can potentially want to style any number of things, but it doesn't seem like a significant use case, to me.
It's obviously pretty harmless to add a class for the year (I don't think I'd use a data attribute for this), but I also don't see a lot of value in it. Could go either way.
There was a problem hiding this comment.
this is less a request, and as @joedolson noted, an opportunity to ask if there’s a reason to add one
| <a href="<?php the_permalink(); ?>"><?php echo esc_html( $title ); ?></a> | ||
| <?php if ( get_current_user_id() !== $author_id ) : ?> | ||
| <span class="wp-on-this-day-post-author"> | ||
| <?php |
There was a problem hiding this comment.
not sure if we intended this, but there’s a whitespace character implied here by the newline. this will return <span> by dmsnell</span> and not <span>by dmsnell</span>
we could capture it into a variable like $escaped_attribution and embed it directly to keep it clear without the space
echo "<span class='wp-on-this-day-post-author'>{$escaped_attribution}</span>";
No, let's keep the scope small right now and just focus on the widget. I want to ensure this lands for 7.1 and the widget alone will be a nice value add. |
|
@dmsnell thank you so much for the feedback implemented some and responded to some. On my end this is good to be merged. I'm happy to iterate on it after merging and own it for a few weeks if we come up with any feedback. |
|
Let's merge it! |
peterwilsoncc
left a comment
peterwilsoncc
left a comment
There was a problem hiding this comment.
I've added quite a few notes inline....
One of my notes is over six parts so it looks larger than it is. With the changes following Matt's comment the display of the widget is much simplified so you can use the native caching in WP_Query instead of caching the markup.
That change removes a bunch of code which is why it's over six different suggestions.
| if ( 2 !== $month || 28 !== $day || $date->format( 'L' ) ) { | ||
| return $clause; | ||
| } | ||
|
|
||
| return array( | ||
| 'relation' => 'OR', | ||
| $clause, | ||
| array( | ||
| 'month' => 2, | ||
| 'day' => 29, | ||
| ), | ||
| ); |
There was a problem hiding this comment.
I usually encourage the early return pattern but I think this is on case in which it could be clearer to add the special case inside the condition.
| if ( 2 !== $month || 28 !== $day || $date->format( 'L' ) ) { | |
| return $clause; | |
| } | |
| return array( | |
| 'relation' => 'OR', | |
| $clause, | |
| array( | |
| 'month' => 2, | |
| 'day' => 29, | |
| ), | |
| ); | |
| // Display leap day posts on Feb 28 in non leap years. | |
| if ( | |
| 28 === $day | |
| && 2 === $month | |
| && false === (bool) $date->format( 'L' ) | |
| ) { | |
| $clause = array( | |
| 'relation' => 'OR', | |
| $clause, | |
| array( | |
| 'month' => 2, | |
| 'day' => 29, | |
| ), | |
| ); | |
| } | |
| return $clause; |
| '%s post has been published in a previous year:', | ||
| '%s posts have been published in previous years:', |
There was a problem hiding this comment.
| '%s post has been published in a previous year:', | |
| '%s posts have been published in previous years:', | |
| '%s post has been published on this day in a previous year:', | |
| '%s posts have been published on this day in previous years:', |
There was a problem hiding this comment.
Why do we need to include the clause "in previous years" - that seems implied to me.
There was a problem hiding this comment.
Agreed. Added "on this day" and removed "in previous years".
| $user_id = get_current_user_id(); | ||
|
|
||
| $cache_key = sprintf( | ||
| 'render_otd_widget:v%d:%d:%s', | ||
| self::CACHE_VERSION, | ||
| $user_id, | ||
| determine_locale() | ||
| ); | ||
| $cache_salt = array( | ||
| current_time( 'Y-m-d' ), | ||
| wp_cache_get_last_changed( 'posts' ), | ||
| ); | ||
|
|
||
| $cached = wp_cache_get_salted( $cache_key, self::CACHE_GROUP, $cache_salt ); | ||
| if ( ! is_string( $cached ) ) { | ||
| $posts = self::get_cached_posts( $user_id ); | ||
|
|
||
| if ( empty( $posts ) ) { | ||
| return; | ||
| } | ||
|
|
||
| ob_start(); | ||
| self::render_posts( $posts ); | ||
| $cached = ob_get_clean(); | ||
|
|
||
| wp_cache_set_salted( $cache_key, $cached, self::CACHE_GROUP, $cache_salt, DAY_IN_SECONDS ); | ||
| } | ||
|
|
||
| echo '<div class="wp-on-this-day-widget">'; | ||
| // Already escaped at write time by the render_* methods below. | ||
| echo $cached; // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped | ||
| echo '</div>'; |
There was a problem hiding this comment.
This is part one of six.
Following the change to a list of posts, the render of the widget is now far less complex so I think it's safe to use the native caching in WP_Query rather than set up a specific cache for the widget. This will provide a couple of advantages:
- on multi-user blogs the same cache will be hit be all users
- The main WP_Query cache is for a very small amount of data -- in this widget it will be only the cache IDs.
- The cache will be stored in the
post-queriesgroup so caching plugins with group flushing will handle it by default.
The docblock will need to be updated accordingly.
| $user_id = get_current_user_id(); | |
| $cache_key = sprintf( | |
| 'render_otd_widget:v%d:%d:%s', | |
| self::CACHE_VERSION, | |
| $user_id, | |
| determine_locale() | |
| ); | |
| $cache_salt = array( | |
| current_time( 'Y-m-d' ), | |
| wp_cache_get_last_changed( 'posts' ), | |
| ); | |
| $cached = wp_cache_get_salted( $cache_key, self::CACHE_GROUP, $cache_salt ); | |
| if ( ! is_string( $cached ) ) { | |
| $posts = self::get_cached_posts( $user_id ); | |
| if ( empty( $posts ) ) { | |
| return; | |
| } | |
| ob_start(); | |
| self::render_posts( $posts ); | |
| $cached = ob_get_clean(); | |
| wp_cache_set_salted( $cache_key, $cached, self::CACHE_GROUP, $cache_salt, DAY_IN_SECONDS ); | |
| } | |
| echo '<div class="wp-on-this-day-widget">'; | |
| // Already escaped at write time by the render_* methods below. | |
| echo $cached; // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped | |
| echo '</div>'; | |
| echo '<div class="wp-on-this-day-widget">'; | |
| self::render_posts( self::get_posts_on_this_day() ); | |
| echo '</div>'; |
| * @param int $user_id Current user ID for filter context. | ||
| * @return WP_Post[] Array of posts ordered by newest first. | ||
| */ | ||
| public static function get_posts_on_this_day( $user_id ) { |
There was a problem hiding this comment.
Part two of six
| * @param int $user_id Current user ID for filter context. | |
| * @return WP_Post[] Array of posts ordered by newest first. | |
| */ | |
| public static function get_posts_on_this_day( $user_id ) { | |
| * @return WP_Post[] Array of posts ordered by newest first. | |
| */ | |
| public static function get_posts_on_this_day() { |
| * @param array $args WP_Query arguments. | ||
| * @param int $user_id Current user ID. | ||
| */ | ||
| $args = apply_filters( 'wp_dashboard_on_this_day_query_args', $args, $user_id ); |
There was a problem hiding this comment.
Part three of six
No longer used.
| * @param array $args WP_Query arguments. | |
| * @param int $user_id Current user ID. | |
| */ | |
| $args = apply_filters( 'wp_dashboard_on_this_day_query_args', $args, $user_id ); | |
| * @param array $args WP_Query arguments. | |
| */ | |
| $args = apply_filters( 'wp_dashboard_on_this_day_query_args', $args ); |
| /** | ||
| * Cache version. Bump when the rendered markup changes so stale | ||
| * entries from older releases are naturally ignored. | ||
| * | ||
| * @since 7.1.0 | ||
| * @var int | ||
| */ | ||
| const CACHE_VERSION = 6; | ||
|
|
There was a problem hiding this comment.
Part six of six
No longer used.
| /** | |
| * Cache version. Bump when the rendered markup changes so stale | |
| * entries from older releases are naturally ignored. | |
| * | |
| * @since 7.1.0 | |
| * @var int | |
| */ | |
| const CACHE_VERSION = 6; |
| /** | ||
| * Object cache group used for storing widget results. | ||
| * | ||
| * @since 7.1.0 | ||
| * @var string | ||
| */ | ||
| const CACHE_GROUP = 'on_this_day'; | ||
|
|
There was a problem hiding this comment.
Part five of six
| /** | |
| * Object cache group used for storing widget results. | |
| * | |
| * @since 7.1.0 | |
| * @var string | |
| */ | |
| const CACHE_GROUP = 'on_this_day'; |
| if ( current_user_can( 'edit_posts' ) ) { | ||
| if ( ! class_exists( 'WP_Dashboard_Widget_On_This_Day' ) ) { | ||
| require_once ABSPATH . 'wp-admin/includes/class-wp-dashboard-widget-on-this-day.php'; | ||
| } | ||
|
|
||
| WP_Dashboard_Widget_On_This_Day::register_widget(); | ||
| } |
There was a problem hiding this comment.
The widget can be displayed to all users now it's blog wide. Similar to the activity widget.
| if ( current_user_can( 'edit_posts' ) ) { | |
| if ( ! class_exists( 'WP_Dashboard_Widget_On_This_Day' ) ) { | |
| require_once ABSPATH . 'wp-admin/includes/class-wp-dashboard-widget-on-this-day.php'; | |
| } | |
| WP_Dashboard_Widget_On_This_Day::register_widget(); | |
| } | |
| if ( ! class_exists( 'WP_Dashboard_Widget_On_This_Day' ) ) { | |
| require_once ABSPATH . 'wp-admin/includes/class-wp-dashboard-widget-on-this-day.php'; | |
| } | |
| WP_Dashboard_Widget_On_This_Day::register_widget(); |
| sprintf( | ||
| /* translators: %s: Post author's display name. */ | ||
| __( 'by %s' ), | ||
| get_the_author() |
There was a problem hiding this comment.
There are a few edge cases here:
- get_the_author() can return an empty value if the display name is not set so you should either fallback to their user name or nice name.
- It's possible for a post not to have an author set, ie
post_author === 0, in which case you can skipe displaying the author
| ) | ||
| ); | ||
|
|
||
| echo '<span class="wp-on-this-day-post-author">' . $escaped_attribution . '</span>'; |
There was a problem hiding this comment.
Remove early escaping and escape upon echo. Escaping late is safer than escaping early as it protects developers from their later selves.
| echo '<span class="wp-on-this-day-post-author">' . $escaped_attribution . '</span>'; | |
| echo '<span class="wp-on-this-day-post-author">' . esc_html( $escaped_attribution ) . '</span>'; |
t-hamano
left a comment
t-hamano
left a comment
There was a problem hiding this comment.
Regarding the direction of this feature, I don't have a strong opinion, but from a code quality perspective, I feel there is still much room for improvement.
There was a problem hiding this comment.
Why does this widget require its own dedicated CSS file? Can't it be integrated into dashboard.css?
There was a problem hiding this comment.
Fully agree with this.there's no reason not to combine this with existing CSS.
| --wp-otd-widget-accent-rgb: var(--wp-admin-theme-color--rgb, 34, 113, 177); | ||
| --wp-otd-widget-muted: #646970; | ||
| --wp-otd-widget-pill: 9999px; | ||
| --wp-otd-widget-accent-dark: var(--wp-admin-theme-color-darker-10, #135e96); | ||
| --wp-otd-widget-accent-8: rgba(var(--wp-otd-widget-accent-rgb), 0.08); |
There was a problem hiding this comment.
I don't think we need to define CSS custom properties that are only used once. It should be sufficient to apply the styles directly inline.
| } | ||
|
|
||
| #wp_dashboard_on_this_day .wp-on-this-day-title::after { | ||
| content: attr(data-wp-otd-window-label); |
There was a problem hiding this comment.
We need to check if placing content with meaning in the CSS content property causes any accessibility issues. Will all screen readers announce this date correctly?
There was a problem hiding this comment.
Yes, screen readers will announce generated content. This still creates a problem; it'll be announced as, e.g. On this dayJULY 6th". However, I don't really see why we would choose to do it this way; what's the barrier to having this directly in the HTML?
| #wp_dashboard_on_this_day .wp-on-this-day-year-heading { | ||
| margin: 0 0 8px; | ||
| font-size: 14px; | ||
| font-weight: 600; | ||
| line-height: 1.4; | ||
| } |
There was a problem hiding this comment.
| #wp_dashboard_on_this_day .wp-on-this-day-year-heading { | |
| margin: 0 0 8px; | |
| font-size: 14px; | |
| font-weight: 600; | |
| line-height: 1.4; | |
| } | |
| #wp_dashboard_on_this_day .wp-on-this-day-year-heading { | |
| font-weight: 600; | |
| } |
Remove redundant CSS overrides
| background: var(--wp-otd-widget-accent-8); | ||
| border-radius: var(--wp-otd-widget-pill); | ||
| white-space: nowrap; | ||
| vertical-align: 1px; |
There was a problem hiding this comment.
Let's avoid the approach of using vertical-align to align elements in new code going forward. This is an outdated method, and we should be able to use a flexbox layout.
| * @since 7.1.0 | ||
| * @var int | ||
| */ | ||
| const POSTS_PER_PAGE = -1; |
There was a problem hiding this comment.
Is this appropriate? If there are a large number of posts on the same day in the past, won't they all be displayed on the dashboard, making the entire dashboard screen very long?
| * | ||
| * @since 7.1.0 | ||
| */ | ||
| class WP_Dashboard_Widget_On_This_Day { |
There was a problem hiding this comment.
I don't understand why only this widget is implemented as a class. Shouldn't this be a function according to other implementations?
- At a Glance →
wp_dashboard_right_now() - Activity →
wp_dashboard_site_activity() - Quick Draft →
wp_dashboard_quick_press() - Events and News →
wp_dashboard_events_news() - Site Health →
wp_dashboard_site_health()
If we were to truly implement this widget as a class, I believe we should evaluate all its methods and avoid making them unnecessarily public.
There was a problem hiding this comment.
Yeah, I share this concern.
| * @since 7.1.0 | ||
| * @var int | ||
| */ | ||
| const CACHE_VERSION = 6; |
There was a problem hiding this comment.
Why is version control necessary for caches? Aren't caches reset every 24 hours?
| * @since 7.1.0 | ||
| * @var int | ||
| */ | ||
| const POSTS_PER_PAGE = -1; |
There was a problem hiding this comment.
Oh, forgot to log this. Unlimited queries can be bad so let's use a high default that's not unlimited.
(In a previous job I worked at a newspaper publishing hundreds of articles a day. They've been using WordPress as their CMS since 2016 so an unlimited query would end up returning at least 1000 articles. This would cause problems. ;)
| const POSTS_PER_PAGE = -1; | |
| const POSTS_PER_PAGE = 50; |
There was a problem hiding this comment.
Do you think this should be a smaller limit? From a practical standpoint, how many posts would somebody actually want to see in this widget? Once we're deciding not to just show them all, it seems like we should set more browseable limit, e.g. the top 10 posts.
There was a problem hiding this comment.
@joedolson Yes, I think you are right. 50 would still make for a very long widget. Using the default value makes sense.
joedolson
left a comment
joedolson
left a comment
There was a problem hiding this comment.
Agree with previous reviews; there's a fair amount of room here to continue to improve.
There was a problem hiding this comment.
Fully agree with this.there's no reason not to combine this with existing CSS.
| } | ||
|
|
||
| #wp_dashboard_on_this_day .wp-on-this-day-title::after { | ||
| content: attr(data-wp-otd-window-label); |
There was a problem hiding this comment.
Yes, screen readers will announce generated content. This still creates a problem; it'll be announced as, e.g. On this dayJULY 6th". However, I don't really see why we would choose to do it this way; what's the barrier to having this directly in the HTML?
| * @since 7.1.0 | ||
| * @var int | ||
| */ | ||
| const POSTS_PER_PAGE = -1; |
There was a problem hiding this comment.
Do you think this should be a smaller limit? From a practical standpoint, how many posts would somebody actually want to see in this widget? Once we're deciding not to just show them all, it seems like we should set more browseable limit, e.g. the top 10 posts.
| * Builds the date query clause for today's anniversary date. | ||
| * | ||
| * On February 28 in a non-leap year, February 29 posts are included so | ||
| * leap-day anniversaries still appear. |
There was a problem hiding this comment.
The logic here makes sense to me, per @alshakero's response.
| '%s post has been published in a previous year:', | ||
| '%s posts have been published in previous years:', |
There was a problem hiding this comment.
Why do we need to include the clause "in previous years" - that seems implied to me.
| <ul class="wp-on-this-day-years"> | ||
| <?php foreach ( $posts_by_year as $year => $year_posts ) : ?> | ||
| <li class="wp-on-this-day-year"> | ||
| <h3 class="wp-on-this-day-year-heading"><?php echo esc_html( $year ); ?></h3> |
There was a problem hiding this comment.
I think @dmsnell is just asking whether it might make sense to add an attribute that extenders can use for styling.
I'm not sure I can see a lot of reason for that; obviously people can potentially want to style any number of things, but it doesn't seem like a significant use case, to me.
It's obviously pretty harmless to add a class for the year (I don't think I'd use a data attribute for this), but I also don't see a lot of value in it. Could go either way.
| ?> | ||
| <li class="wp-on-this-day-post"> | ||
| <a href="<?php the_permalink(); ?>"><?php echo esc_html( $title ); ?></a> | ||
| <?php if ( get_current_user_id() !== $author_id ) : ?> |
There was a problem hiding this comment.
I don't see a lot of value in omitting the current user from this data. For single author sites it makes sense, as all authors are the same person; but for heavily multi-author sites it just looks strange to me for some posts to omit author information.
There was a problem hiding this comment.
Yes but the vast majority of sites are single-author, so this will be noise for most people.
| } | ||
|
|
||
| // On This Day. | ||
| if ( current_user_can( 'edit_posts' ) ) { |
There was a problem hiding this comment.
Agreed. There's nothing non-public about this.
| wp_add_dashboard_widget( | ||
| self::WIDGET_ID, | ||
| sprintf( | ||
| '<span class="wp-on-this-day-title" data-wp-otd-window-label="%s">%s</span>', |
There was a problem hiding this comment.
I don't understand why we aren't rendering the date directly in HTML. It makes the text unselectable.
I assume it's to avoid problems with justify-content: space-between on the heading? That can be gotten around by nesting elements, too, so it's not necessary.
I'm just not sure we should be making it habitual to use CSS generated readable content.
| * | ||
| * @since 7.1.0 | ||
| */ | ||
| public static function register_widget() { |
There was a problem hiding this comment.
It calls through to wp_add_dashboard_widget, so I think it should follow a similar naming convention. add_dashboard_widget or even just add.
Also it'd be good to disambiguate from the existing register_widget - https://developer.wordpress.org/reference/functions/register_widget/, as that's for the other type of widget.
| * | ||
| * @since 7.1.0 | ||
| */ | ||
| public static function render_dashboard_widget() { |
There was a problem hiding this comment.
Does it need to be public?
If it's this way for the test, then I think there are examples across that codebase of using reflection for this.
|
@alshakero, Do you have the bandwidth to address the feedback? If not, someone else can help with the task 🙂 |
|
Hi @t-hamano! I'm on this now. I was on a short holiday. |
|
Thank you for the feedback. I admit, this is much neater now. Summary of the changes: @peterwilsoncc's review:
@t-hamano's review:
@joedolson's comments:
|
18 participants
Summary
Adds a new On This Day dashboard widget to WordPress core that surfaces the current user's posts published on today's month and day in previous years, so returning authors see a friendly nudge of what they wrote one, five, or ten years ago.
The widget is implemented as a first-class core feature, following the same pattern as Site Health.
User-facing behavior
On This Day · <Month Day>and appears in the dashboard grid for users withedit_posts.2023 · 3 yrs) and the posts published that day as cards with excerpt, time, categories, and Edit/View links.Viewlink to the permalink.Screenshots
Testing
npm run env:start, thennpm run env:install.Trac ticket: https://core.trac.wordpress.org/ticket/65116#ticket
Use of AI Tools
AI assistance: Yes
Tool(s): Cursor
Model(s): Claude Opus 4.7 + Codex 5.5
Used for: The code is ~50% written with AI, but I guided it every step of the way and reviewed every line.
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.