Now the date will work first time. If you set the start date after th…#50
Now the date will work first time. If you set the start date after th…#50
Conversation
…e end date, the end date is set 1 hour after the start date. if end date is set before the start date, the start date is updated to 1hr before the end time
WalkthroughThese changes modify how date/time values and their display conditions are handled. In the event info component, the derivation of newDate/newTime is unified through a consolidated dateInstance that prioritizes provided temp values, falls back to dateInstance if present, and finally defaults to previous start/end values. In the date display formatter, the all-day label visibility is now conditional on the date_only setting, and timezone abbreviations are displayed only when display_timezone is true, time_only is false, and the event is not all-day. Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/classes/class-date-display-formatter.php (1)
706-708: Timezone display gating looks good; consider DST‑accurate abbreviationCondition now avoids showing TZ for all‑day and time‑only—nice. For better DST accuracy, derive the abbreviation from the event’s timestamp rather than current time:
private function get_timezone_abbreviation_for( int $timestamp ) { $dt = new DateTime( '@' . $timestamp ); $dt->setTimezone( $this->get_timezone_instance() ); return $dt->format( 'T' ); }Then call with the displayed boundary (e.g., start_date).
If you prefer to keep the current helper, verify an event spanning a DST boundary shows the expected abbreviation for its start.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/blocks/event-info/components/DateTimeGroup.js(1 hunks)src/classes/class-date-display-formatter.php(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/classes/class-date-display-formatter.php (1)
src/classes/class-se-settings.php (1)
get_all_day_message(819-826)
src/blocks/event-info/components/DateTimeGroup.js (1)
src/blocks/event-info/date-utils.js (2)
eventStart(241-241)eventEnd(246-246)
| // Get the full date time instance from either temp date or time. | ||
| const dateInstance = tempEventDate ? tempEventDate : tempEventTime; | ||
|
|
||
| // Set the new date and time based on what was changed. | ||
| const newDate = tempEventDate || dateInstance || (isStartChange ? eventStart : eventEnd); | ||
| const newTime = tempEventTime || dateInstance || (isStartChange ? eventStart : eventEnd); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Guard against overwriting the untouched part when only date OR time changes
Using dateInstance for both newDate and newTime can unintentionally reset the untouched part (e.g., date-only change setting time to midnight, or time-only change injecting an unintended date), depending on what DateTimePicker returns. Also mixes string vs moment types.
Recommend deriving the untouched half from the existing value for that boundary:
-// Get the full date time instance from either temp date or time.
-const dateInstance = tempEventDate ? tempEventDate : tempEventTime;
-
-// Set the new date and time based on what was changed.
-const newDate = tempEventDate || dateInstance || (isStartChange ? eventStart : eventEnd);
-const newTime = tempEventTime || dateInstance || (isStartChange ? eventStart : eventEnd);
+// Use the current boundary as base; only replace the changed half.
+const base = isStartChange ? eventStart : eventEnd;
+const newDate = tempEventDate || base;
+const newTime = tempEventTime || base;Optionally normalize types before combineDateAndTime to avoid string/moment mixing.
To de-risk, please confirm two points:
And check the WP component’s onChange payload:
🏁 Script executed:
#!/bin/bash
# 1) Inspect combineDateAndTime signature and usage
rg -nC3 "combineDateAndTime" --type=js
# 2) Find where tempEventDate/tempEventTime are set/used
rg -nC3 "tempEvent(Date|Time)" src --type=jsLength of output: 5094
🌐 Web query:
@wordpress/components DateTimePicker: does onChange return a full ISO datetime (date+time) for both date and time interactions?
💡 Result:
Short answer: Yes — onChange receives the component's currentDate value (a string or null) and when the DateTimePicker is used for date+time it will supply the combined datetime (ISO-like string). See the prop docs: onChange: (date: string | null) => void and the component passes the currentDate argument. [1]
Notes:
- The docs also call out that month-preview callbacks use an ISO date string, which implies the component uses ISO-formatted strings for its date callbacks. [1]
- If you need a Date object or a different format, pass a Date (or parse the returned string) or supply currentDate as a Date and handle conversion in your onChange. For isolated time-input behavior there are separate TimePicker changes to be aware of. [2]
Sources:
[1] WordPress Block Editor handbook — DateTime (DateTimePicker) (props: currentDate, onChange, onMonthPreviewed).
[2] Make.WordPress core note — TimePicker / TimeInput behavior.
I need to verify one more critical detail: how datePickerHandler processes the DateTimePicker onChange value. Let me check:
🏁 Script executed:
#!/bin/bash
# Find datePickerHandler function definition
rg -nA15 "const datePickerHandler\s*=" src/blocks/event-info/components/DateTimeGroup.jsLength of output: 89
🏁 Script executed:
#!/bin/bash
# Find datePickerHandler function definition
rg -nA15 "const datePickerHandler\s*=" src/blocks/event-info/components/DateTimeGroup.jsLength of output: 540
Fix the untouched part being overwritten during date-only or time-only changes
The bug is confirmed. When the user edits only the date or time, datePickerHandler stores the full ISO datetime string in either tempEventDate or tempEventTime. Lines 85–90 then mistakenly use that full datetime as a fallback for both newDate and newTime, overwriting the part that wasn't changed.
For example:
- Time-only change:
tempEventDate = null,tempEventTime = "2024-10-22T15:30:00"→newDateincorrectly becomes the full datetime, resetting the date. - Date-only change:
tempEventDate = "2024-10-25T09:00:00",tempEventTime = null→newTimeincorrectly becomes the full datetime, resetting the time.
Replace lines 85–90 with:
-// Get the full date time instance from either temp date or time.
-const dateInstance = tempEventDate ? tempEventDate : tempEventTime;
-
-// Set the new date and time based on what was changed.
-const newDate = tempEventDate || dateInstance || (isStartChange ? eventStart : eventEnd);
-const newTime = tempEventTime || dateInstance || (isStartChange ? eventStart : eventEnd);
+// Use the current boundary as base; preserve the untouched half.
+const base = isStartChange ? eventStart : eventEnd;
+const newDate = tempEventDate || base;
+const newTime = tempEventTime || base;🤖 Prompt for AI Agents
In src/blocks/event-info/components/DateTimeGroup.js around lines 85 to 91, the
fallback logic uses a full ISO datetime stored in tempEventDate or tempEventTime
for both newDate and newTime causing the untouched part to be overwritten;
change the logic to extract and use only the date portion from tempEventDate for
newDate and only the time portion from tempEventTime for newTime, and combine
each with the other part from the existing event (use eventStart/eventEnd
selected by isStartChange) when the counterpart is null — parse the ISO strings,
take the relevant component (date or time), and construct the new datetime
values without using a full datetime from the opposite temp variable as a
fallback.
| // For all day events, just show the date (or date range if different days). | ||
| if ( ! $same_day ) { | ||
| $output .= ' – ' . $end_date . ' ' . SE_Settings::get_all_day_message(); | ||
| } else { | ||
| $output .= ' ' . SE_Settings::get_all_day_message(); | ||
| } elseif ( ! $this->date_only ) { | ||
| $output .= ' ' . SE_Settings::get_all_day_message(); | ||
| } |
There was a problem hiding this comment.
Make all‑day label consistent for multi‑day when date‑only is set
You correctly hide the all‑day label for same‑day when date_only is true, but the multi‑day branch still appends it unconditionally. Align both branches:
- if ( ! $same_day ) {
- $output .= ' – ' . $end_date . ' ' . SE_Settings::get_all_day_message();
- } elseif ( ! $this->date_only ) {
+ if ( ! $same_day ) {
+ $output .= ' – ' . $end_date;
+ if ( ! $this->date_only ) {
+ $output .= ' ' . SE_Settings::get_all_day_message();
+ }
+ } elseif ( ! $this->date_only ) {
$output .= ' ' . SE_Settings::get_all_day_message();
}This preserves “start – end” for date‑only without the label, in both same‑day and multi‑day cases.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // For all day events, just show the date (or date range if different days). | |
| if ( ! $same_day ) { | |
| $output .= ' – ' . $end_date . ' ' . SE_Settings::get_all_day_message(); | |
| } else { | |
| $output .= ' ' . SE_Settings::get_all_day_message(); | |
| } elseif ( ! $this->date_only ) { | |
| $output .= ' ' . SE_Settings::get_all_day_message(); | |
| } | |
| // For all day events, just show the date (or date range if different days). | |
| if ( ! $same_day ) { | |
| $output .= ' – ' . $end_date; | |
| if ( ! $this->date_only ) { | |
| $output .= ' ' . SE_Settings::get_all_day_message(); | |
| } | |
| } elseif ( ! $this->date_only ) { | |
| $output .= ' ' . SE_Settings::get_all_day_message(); | |
| } |
🤖 Prompt for AI Agents
In src/classes/class-date-display-formatter.php around lines 675 to 680, the
multi-day branch always appends the all-day label even when $this->date_only is
true; update the conditional so both branches respect date_only: when not
same_day and date_only is true, append only the end date (no all-day message),
otherwise append the end date plus SE_Settings::get_all_day_message(); keep the
existing same-day elseif as-is so both paths consistently suppress the all-day
label when date_only is set.
1 participant
…e end date, the end date is set 1 hour after the start date. if end date is set before the start date, the start date is updated to 1hr before the end time.
Add a few pending changes around how we display timezones
Changes proposed in this Pull Request
Ensure that the fully date/time instance is use when we first set the date time.
Testing instructions
Mentions #49
Summary by CodeRabbit
Bug Fixes
Refactor