Update Service Request question to new design #15215
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds three English localization keys for service-request selection and refactors the ServiceRequestQuestion UI: swaps several UI primitives, replaces the priority select with radio inputs, introduces a pickerSlot for activity definition selection, restructures badges/layout, and updates state and event wiring. Changes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR updates the Service Request question component to align with a new design system. The changes modernize the UI with improved visual hierarchy, better component organization, and enhanced user experience through clearer information presentation and improved interaction patterns.
Changes:
- Replaced Select component with RadioInput for priority selection, improving accessibility and clarity
- Replaced standard Textarea with AutoExpandingTextarea for better text input experience
- Updated visual design with new badge variants, separators, and consistent spacing
- Reorganized layout from grid-based to stacked vertical layout for better responsiveness
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/components/Questionnaire/QuestionTypes/ServiceRequestQuestion.tsx | Refactored component to use new design system with updated components, improved layout structure, and enhanced visual hierarchy |
| public/locale/en.json | Added new translation keys for the updated service request selection UI |
| <div className="w-px h-10 bg-gray-300 self-center" /> | ||
| <div className="flex flex-col gap-1"> | ||
| <span className="text-xs font-normal leading-none text-gray-700"> | ||
| |
There was a problem hiding this comment.
Using a non-breaking space character as placeholder text is unclear and makes the code harder to maintain. Consider either removing this span element entirely if the vertical alignment works without it, or use an explicit empty string with a comment explaining why the spacing is needed for visual alignment with other badges.
| | |
| {/* Intentionally left empty to align this badge with other labeled badges */} | |
| {""} |
| {t("cancel")} | ||
| </Button> | ||
| )} | ||
| <Button variant="outline_primary" onClick={onAdd} className=""> |
There was a problem hiding this comment.
Empty className prop serves no purpose and should be removed for cleaner code.
| <Button variant="outline_primary" onClick={onAdd} className=""> | |
| <Button variant="outline_primary" onClick={onAdd}> |
Deploying care-preview with
|
| Latest commit: |
c50fb13
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://b5b42c82.care-preview-a7w.pages.dev |
| Branch Preview URL: | https://implement-new-sr-design.care-preview-a7w.pages.dev |
🎭 Playwright Test ResultsStatus: ❌ Failed
📊 Detailed results are available in the playwright-final-report artifact. Run: #4909 |
| "select_section_to_add": "Select Section to Add", | ||
| "select_service_account_type": "Select service account type", | ||
| "select_service_point": "Select Service Point", | ||
| "select_service_request": "Select the service request", |
There was a problem hiding this comment.
The translation key contains an unnecessary article 'the'. It should be "Select service request" to match the pattern of similar keys like "select_another_service_request" and maintain consistency with other selection prompts in the file.
| "select_service_request": "Select the service request", | |
| "select_service_request": "Select service request", |
| <span className="text-xs font-normal leading-none text-gray-700"> | ||
| |
There was a problem hiding this comment.
Using a non-breaking space as placeholder content is a code smell. Consider using an empty string or CSS styling (min-height) to maintain spacing instead of relying on invisible content.
| <span className="text-xs font-normal leading-none text-gray-700"> | |
| | |
| <span className="text-xs font-normal leading-none text-gray-700 min-h-[1rem]"> | |
| {""} |
| } else { | ||
| setSelectedActivityDefinition(def.slug); | ||
| } |
There was a problem hiding this comment.
The early return pattern would be cleaner here. Consider restructuring to handle the null case first and return early, then handle the success case without the else block.
| } else { | |
| setSelectedActivityDefinition(def.slug); | |
| } | |
| return; | |
| } | |
| setSelectedActivityDefinition(def.slug); |
ohc-project-manager
bot
added
needs testing
needs review
and removed
Work in Progress
labels
| "select_section_to_add": "Select Section to Add", | ||
| "select_service_account_type": "Select service account type", | ||
| "select_service_point": "Select Service Point", | ||
| "select_service_request": "Select the service request", |
There was a problem hiding this comment.
Corrected article usage from 'the' to 'a' for more natural phrasing in selection prompts.
| "select_service_request": "Select the service request", | |
| "select_service_request": "Select a service request", |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/Questionnaire/QuestionTypes/ServiceRequestQuestion.tsx (1)
356-429: Significant code duplication between preview and collapsed form views.The form fields (priority, body_site, patient_instruction, requester, note) are nearly identical between the preview (lines 229-300) and collapsed (lines 356-428) layouts. Consider extracting these into a shared helper component or function to reduce duplication and ensure consistency.
Additionally, there's a minor styling inconsistency: the preview AutoExpandingTextarea includes
text-smclass (lines 286, 297) while the collapsed view does not (lines 405, 426).♻️ Suggested approach
Extract the form fields into a reusable component:
function ServiceRequestFormFields({ serviceRequest, onUpdate, disabled, errors, questionId, index, facilityId, }: { serviceRequest: ServiceRequestApplyActivityDefinitionSpec; onUpdate?: (updates: Partial<ServiceRequestReadSpec>) => void; disabled?: boolean; errors?: QuestionValidationError[]; questionId?: string; index?: number; facilityId?: string; }) { const { t } = useTranslation(); return ( <div className="space-y-4"> {/* Priority, body_site, requester, patient_instruction, note fields */} </div> ); }Then use it in both preview and collapsed views.
🤖 Fix all issues with AI agents
In `@src/components/Questionnaire/QuestionTypes/ServiceRequestQuestion.tsx`:
- Around line 673-716: The ResourceDefinitionCategoryPicker props are
duplicated; extract the shared config into a single const (e.g.,
activityDefinitionPickerProps) containing facilityId, value
(selectedActivityDefinitionData || undefined), onValueChange
(handleActivityDefinitionSelect), disabled, className, resourceType
(ResourceCategoryResourceType.activity_definition), listDefinitions (queryFn:
activityDefinitionApi.listActivityDefinition, pathParams: { facilityId }), and
translationBaseKey, then spread that object into both
<ResourceDefinitionCategoryPicker<ActivityDefinitionReadSpec>> usages and only
override the differing placeholder strings via a placeholder prop.
- Around line 696-702: The i18n key is being passed via a ternary inside the t()
call in the JSX span in ServiceRequestQuestion (the expression using
t(serviceRequests.length > 0 ? "select_another_service_request" :
"select_service_request")), which prevents static key detection; fix it by
removing the ternary and calling t() separately for each branch (e.g., compute
label = serviceRequests.length > 0 ? t("select_another_service_request") :
t("select_service_request") or render {serviceRequests.length > 0 ?
t("select_another_service_request") : t("select_service_request")}) so both keys
are statically visible to the i18n cleanup script, referencing the span element,
t(), and serviceRequests.length in ServiceRequestQuestion.
- Around line 333-336: The CareIcon usage inside ServiceRequestQuestion (the
CareIcon component instance with icon="l-edit" and className="size-5
text-gray-950 font-normal") should be made accessible: if the icon is purely
decorative add aria-hidden="true" and role="presentation" to the CareIcon props;
if it conveys meaning for screen readers provide a concise aria-label (e.g.,
aria-label="Edit" or aria-label="Expand section") instead and do not mark it
hidden. Update the CareIcon props accordingly in the ServiceRequestQuestion
component.
♻️ Duplicate comments (3)
public/locale/en.json (1)
4928-4928: Align wording with other selection prompts.Consider removing the article to match similar keys like "select_another_service_request".
💬 Suggested tweak
- "select_service_request": "Select the service request", + "select_service_request": "Select service request",src/components/Questionnaire/QuestionTypes/ServiceRequestQuestion.tsx (2)
204-209: Non-breaking space used for visual alignment is a code smell.Using
as placeholder content for alignment is fragile and unclear. Consider using CSS min-height or removing the span if the layout works without it.
607-614: Early return pattern would be cleaner.Consider restructuring to handle the null case first with an early return, then handle the success case without the else block.
| <CareIcon | ||
| icon="l-edit" | ||
| className="size-5 text-gray-950 font-normal" | ||
| /> |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding aria-label for the edit icon.
The edit icon appears to convey that the collapsible section is expandable/editable. Adding an aria-hidden="true" attribute would explicitly mark it as decorative, or add an aria-label if it should convey meaning to screen readers.
<CareIcon
icon="l-edit"
className="size-5 text-gray-950 font-normal"
+ aria-hidden="true"
/>📝 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.
| <CareIcon | |
| icon="l-edit" | |
| className="size-5 text-gray-950 font-normal" | |
| /> | |
| <CareIcon | |
| icon="l-edit" | |
| className="size-5 text-gray-950 font-normal" | |
| aria-hidden="true" | |
| /> |
🤖 Prompt for AI Agents
In `@src/components/Questionnaire/QuestionTypes/ServiceRequestQuestion.tsx` around
lines 333 - 336, The CareIcon usage inside ServiceRequestQuestion (the CareIcon
component instance with icon="l-edit" and className="size-5 text-gray-950
font-normal") should be made accessible: if the icon is purely decorative add
aria-hidden="true" and role="presentation" to the CareIcon props; if it conveys
meaning for screen readers provide a concise aria-label (e.g., aria-label="Edit"
or aria-label="Expand section") instead and do not mark it hidden. Update the
CareIcon props accordingly in the ServiceRequestQuestion component.
| pickerSlot={ | ||
| <ResourceDefinitionCategoryPicker<ActivityDefinitionReadSpec> | ||
| facilityId={facilityId} | ||
| value={selectedActivityDefinitionData || undefined} | ||
| onValueChange={handleActivityDefinitionSelect} | ||
| placeholder={t("select_activity_definition")} | ||
| disabled={disabled} | ||
| className="w-full" | ||
| resourceType={ResourceCategoryResourceType.activity_definition} | ||
| listDefinitions={{ | ||
| queryFn: activityDefinitionApi.listActivityDefinition, | ||
| pathParams: { facilityId }, | ||
| }} | ||
| translationBaseKey="activity_definition" | ||
| /> | ||
| } | ||
| /> | ||
| )} | ||
|
|
||
| <div className="space-y-2 w-full"> | ||
| <ResourceDefinitionCategoryPicker<ActivityDefinitionReadSpec> | ||
| facilityId={facilityId} | ||
| value={selectedActivityDefinitionData || undefined} | ||
| onValueChange={handleActivityDefinitionSelect} | ||
| placeholder={t("select_activity_definition")} | ||
| disabled={disabled} | ||
| className="w-full" | ||
| resourceType={ResourceCategoryResourceType.activity_definition} | ||
| listDefinitions={{ | ||
| queryFn: activityDefinitionApi.listActivityDefinition, | ||
| pathParams: { facilityId }, | ||
| }} | ||
| translationBaseKey="activity_definition" | ||
| /> | ||
| </div> | ||
| {!previewServiceRequest && !isLoadingSelectedAD && ( | ||
| <div className="rounded-lg border border-gray-200 bg-gray-200/20 shadow-sm overflow-hidden relative"> | ||
| <div className="absolute left-0 top-4 w-1 h-6 bg-indigo-600 rounded-r-full" /> | ||
| <div className="p-4"> | ||
| <span className="text-lg font-semibold leading-6 text-gray-900"> | ||
| {t( | ||
| serviceRequests.length > 0 | ||
| ? "select_another_service_request" | ||
| : "select_service_request", | ||
| )} | ||
| </span> | ||
| <ResourceDefinitionCategoryPicker<ActivityDefinitionReadSpec> | ||
| facilityId={facilityId} | ||
| value={selectedActivityDefinitionData || undefined} | ||
| onValueChange={handleActivityDefinitionSelect} | ||
| placeholder={t("select_from_list")} | ||
| disabled={disabled} | ||
| className="w-full" | ||
| resourceType={ResourceCategoryResourceType.activity_definition} | ||
| listDefinitions={{ | ||
| queryFn: activityDefinitionApi.listActivityDefinition, | ||
| pathParams: { facilityId }, | ||
| }} | ||
| translationBaseKey="activity_definition" | ||
| /> |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider extracting shared picker props to reduce duplication.
The ResourceDefinitionCategoryPicker configuration is duplicated between the preview mode (lines 674-687) and the standalone picker (lines 703-716). While functional, extracting shared props would improve maintainability.
♻️ Optional refactor
const activityDefinitionPickerProps = {
facilityId,
value: selectedActivityDefinitionData || undefined,
onValueChange: handleActivityDefinitionSelect,
disabled,
className: "w-full",
resourceType: ResourceCategoryResourceType.activity_definition,
listDefinitions: {
queryFn: activityDefinitionApi.listActivityDefinition,
pathParams: { facilityId },
},
translationBaseKey: "activity_definition" as const,
};
// Then use:
<ResourceDefinitionCategoryPicker<ActivityDefinitionReadSpec>
{...activityDefinitionPickerProps}
placeholder={t("select_activity_definition")}
/>🤖 Prompt for AI Agents
In `@src/components/Questionnaire/QuestionTypes/ServiceRequestQuestion.tsx` around
lines 673 - 716, The ResourceDefinitionCategoryPicker props are duplicated;
extract the shared config into a single const (e.g.,
activityDefinitionPickerProps) containing facilityId, value
(selectedActivityDefinitionData || undefined), onValueChange
(handleActivityDefinitionSelect), disabled, className, resourceType
(ResourceCategoryResourceType.activity_definition), listDefinitions (queryFn:
activityDefinitionApi.listActivityDefinition, pathParams: { facilityId }), and
translationBaseKey, then spread that object into both
<ResourceDefinitionCategoryPicker<ActivityDefinitionReadSpec>> usages and only
override the differing placeholder strings via a placeholder prop.
| <span className="text-lg font-semibold leading-6 text-gray-900"> | ||
| {t( | ||
| serviceRequests.length > 0 | ||
| ? "select_another_service_request" | ||
| : "select_service_request", | ||
| )} | ||
| </span> |
There was a problem hiding this comment.
Ternary inside t() may prevent i18n key detection by cleanup scripts.
Per project conventions, the i18n cleanup script cannot detect keys wrapped in ternary expressions passed directly to t(). Use separate t() calls for each branch to ensure all keys are statically detectable.
Based on learnings, this pattern should be avoided.
🔧 Proposed fix
- <span className="text-lg font-semibold leading-6 text-gray-900">
- {t(
- serviceRequests.length > 0
- ? "select_another_service_request"
- : "select_service_request",
- )}
- </span>
+ <span className="text-lg font-semibold leading-6 text-gray-900">
+ {serviceRequests.length > 0
+ ? t("select_another_service_request")
+ : t("select_service_request")}
+ </span>📝 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.
| <span className="text-lg font-semibold leading-6 text-gray-900"> | |
| {t( | |
| serviceRequests.length > 0 | |
| ? "select_another_service_request" | |
| : "select_service_request", | |
| )} | |
| </span> | |
| <span className="text-lg font-semibold leading-6 text-gray-900"> | |
| {serviceRequests.length > 0 | |
| ? t("select_another_service_request") | |
| : t("select_service_request")} | |
| </span> |
🤖 Prompt for AI Agents
In `@src/components/Questionnaire/QuestionTypes/ServiceRequestQuestion.tsx` around
lines 696 - 702, The i18n key is being passed via a ternary inside the t() call
in the JSX span in ServiceRequestQuestion (the expression using
t(serviceRequests.length > 0 ? "select_another_service_request" :
"select_service_request")), which prevents static key detection; fix it by
removing the ternary and calling t() separately for each branch (e.g., compute
label = serviceRequests.length > 0 ? t("select_another_service_request") :
t("select_service_request") or render {serviceRequests.length > 0 ?
t("select_another_service_request") : t("select_service_request")}) so both keys
are statically visible to the i18n cleanup script, referencing the span element,
t(), and serviceRequests.length in ServiceRequestQuestion.
|
Conflicts have been detected against the base branch. Please merge the base branch into your branch.
|
1 similar comment
|
Conflicts have been detected against the base branch. Please merge the base branch into your branch.
|
Proposed Changes
Fixes #15216
Screen.Recording.2026-01-19.014323.mp4
Tagging: @ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
New Features
Improvements
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.