Fix: Resource Letter printing issue #10230
Conversation
WalkthroughThe pull request introduces significant changes to the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Deploy Preview for care-ohc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/components/Resource/ResourceDetails.tsx (2)
106-106: Improve type safety for translation function parameter.The type annotation
(arg0: string) => stringfor the translation function is oversimplified and doesn't account for i18next's interpolation capabilities.Consider using the proper i18next types:
-const RequestLetter = (data: ResourceRequest, t: (arg0: string) => string) => { +import { TFunction } from 'i18next'; +const RequestLetter = (data: ResourceRequest, t: TFunction) => {
384-384: Consider using object destructuring for better maintainability.Using positional parameters can be error-prone when adding or reordering parameters in the future.
Consider using object destructuring:
-{RequestLetter(data, t)} +{RequestLetter({ data, t })}And update the function signature accordingly:
const RequestLetter = ({ data, t }: { data: ResourceRequest; t: TFunction }) => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Resource/ResourceDetails.tsx(2 hunks)
🔇 Additional comments (1)
src/components/Resource/ResourceDetails.tsx (1)
Line range hint
106-384: LGTM! The changes effectively address the resource letter printing issue.The implementation properly integrates translation capabilities into the letter component, ensuring that all static text can be properly translated when printing. The changes maintain consistency with the application's internationalization approach.
There was a problem hiding this comment.
This solves the immediate problem, however print doesn't work as intended for request letter (with print preview displaying RequestLetter multiple times).
- You would need to convert RequestLetter (prob rename to PrintRequestLetter) into its own component and have its content wrapped in PrintPreview
- add a new /print route that calls this component
- update Resourcedetails (buttons, state etc)
- replace useTanStackQueryInstead while you are at it.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/components/Resource/ResourceDetails.tsx (2)
1-1: Great adoption ofuseQuery!
Switching to the official@tanstack/react-queryhook improves maintainability and consistency. However, consider handling potential error states (e.g., network failures) to offer a more robust user experience.Also applies to: 20-20
107-115: Add error handling logic.
Currently,isLoadingis checked, but there is no fallback or notification for errors. Consider passingonErroror checking forerrorfromuseQueryto gracefully handle failed requests and provide user feedback.src/components/Resource/PrintResourceLetter.tsx (1)
14-26: Consider handling query errors.
WhileisLoading || !datais checked, handling potentialerrorwould improve resiliency. You can display an error message or route the user back.+ const { data, isLoading, error } = useQuery(...); + if (error) { + return <p className="text-red-600">{t("failed_to_load_resource")}</p>; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Routers/routes/ResourceRoutes.tsx(2 hunks)src/components/Resource/PrintResourceLetter.tsx(1 hunks)src/components/Resource/ResourceDetails.tsx(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: cypress-run (1)
- GitHub Check: OSSAR-Scan
🔇 Additional comments (4)
src/components/Resource/ResourceDetails.tsx (1)
130-130: Navigation approach looks good.
Switching from print mode state to a dedicated print route increases clarity and separation of concerns. Ensure that any downstream users are aware of the new path.src/Routers/routes/ResourceRoutes.tsx (1)
2-2: Route addition is consistent.
ImportingPrintResourceLetterand adding"/resource/:id/print"is a clean design choice. It aligns with best practices for route-based print handling.Also applies to: 14-14
src/components/Resource/PrintResourceLetter.tsx (2)
1-13: Imports are well-organized.
These imports adequately cover the new component’s requirements, including querying, translation, and utility usage.
27-129: Rendering logic is well-structured.
The content is neatly organized, and i18n usage looks comprehensive. The separated print layout offers clarity and maintainability.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/style/index.css (1)
434-436: LGTM! Consider standardizing all page margins.The
margin-top: 0rule will remove unwanted spacing at the top of printed pages. However, for consistency, consider explicitly setting all margin properties.@page { - margin-top: 0; + margin: 0; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/style/index.css(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: cypress-run (1)
- GitHub Check: OSSAR-Scan
🔇 Additional comments (1)
src/style/index.css (1)
434-436: Verify print layout across browsers.Different browsers may handle
@pagemargins differently. Please test the print layout in major browsers (Chrome, Firefox, Safari) to ensure consistent results.
Proposed Changes
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Refactor