-
-
Notifications
You must be signed in to change notification settings - Fork 226
Improve time offset ux #1772
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve time offset ux #1772
Conversation
📝 WalkthroughWalkthroughAdjusts how time_offset is presented: hides the field for first or sole tasks, pluralizes the seconds label via trans_choice, and updates table rendering to show placeholders or dynamic suffixes based on task sequence. Adds a new translation key for "First Task". Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as Filament TasksRelationManager
participant I18n as Translations
rect rgba(230,240,255,0.5)
note over UI: Form rendering
User->>UI: Open Task Form
UI->>UI: Read schedule task count and task.sequence_id
alt first task or no other tasks
UI->>UI: Hide time_offset input
else subsequent task
UI->>I18n: trans_choice("server/schedule.tasks.seconds", n)
I18n-->>UI: "Second"/"Seconds"
UI->>User: Render time_offset with pluralized suffix
end
end
rect rgba(235,255,235,0.5)
note over UI: Table rendering
User->>UI: View Tasks Table
loop for each task
UI->>UI: Check task.sequence_id
alt sequence_id == 1
UI->>User: Show placeholder / null for time_offset (e.g., "First Task")
else sequence_id > 1
UI->>I18n: trans_choice("server/schedule.tasks.seconds", n)
I18n-->>UI: "Second"/"Seconds"
UI->>User: Show time_offset with suffix
end
end
end
Pre-merge checks✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/Filament/Server/Resources/Schedules/RelationManagers/TasksRelationManager.php (1)
80-80
: Consider using the actual field value for pluralization.The hardcoded count of
2
forces the plural form "Seconds" in all cases. For improved UX, consider using the actualtime_offset
value to display the correct singular/plural form dynamically.Apply this diff to use the actual field value:
- ->suffix(trans_choice('server/schedule.tasks.seconds', 2)), + ->suffix(fn (Get $get) => trans_choice('server/schedule.tasks.seconds', $get('time_offset') ?? 2)),Note: This would require the field to be
live()
for dynamic updates as the user types, which may add complexity. The current approach is acceptable if you prefer simplicity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/Filament/Server/Resources/Schedules/RelationManagers/TasksRelationManager.php
(2 hunks)lang/en/server/schedule.php
(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/Filament/Server/Resources/Schedules/RelationManagers/TasksRelationManager.php (2)
app/Models/Schedule.php (1)
tasks
(133-136)app/Models/Task.php (1)
Task
(27-123)
🔇 Additional comments (4)
lang/en/server/schedule.php (2)
80-80
: LGTM!The new translation key for "First Task" is correctly added and aligns with the PR objective to improve UX for the first task in a schedule.
81-81
: LGTM!The pluralization format using pipe separator (Second|Seconds) is correct for Laravel's
trans_choice()
function. This enables proper singular/plural handling based on the time offset value.app/Filament/Server/Resources/Schedules/RelationManagers/TasksRelationManager.php (2)
75-75
: LGTM!The added condition
$schedule->tasks->count() === 0
correctly hides the time_offset field when creating the first task in an empty schedule. Combined with the existing$get('sequence_id') === 1
check for editing the first task, this properly implements the PR objective to hide the offset field for first tasks.
111-113
: LGTM!The table column logic correctly implements the PR objective:
- Line 111: Applies a pluralized suffix only for non-first tasks, using the actual
time_offset
value for proper singular/plural handling.- Line 112: Returns
null
for the first task to trigger the placeholder, and the actualtime_offset
value for subsequent tasks.- Line 113: Displays "First Task" placeholder when the state is null (first task only).
The logic flow ensures:
- First task displays: "First Task" (no offset shown)
- Subsequent tasks display: "123 seconds" or "1 second" (with proper pluralization)
app/Filament/Server/Resources/Schedules/RelationManagers/TasksRelationManager.php
Outdated
Show resolved
Hide resolved
Co-authored-by: Lance Pioch <[email protected]>
Supersedes #1771