-
Notifications
You must be signed in to change notification settings - Fork 477
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
Dosage heading is misaligned in the prescription module #8645 #8751
Dosage heading is misaligned in the prescription module #8645 #8751
Conversation
✅ Deploy Preview for care-ohc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for care-ohc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
@nihal467 @rithviknishad kindly review the PR . |
src/Components/Medicine/MedicineAdministrationSheet/AdministrationTableRow.tsx
Outdated
Show resolved
Hide resolved
@nihal467 the parts medicine and dosage are not separate table columns but a single div with "justify-between" . Do you want me to change them to separate table columns? |
Yup, let's try that. |
👋 Hi, @Sulochan-khadka, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
Here is the improvement @nihal467 @rithviknishad |
@Sulochan-khadka can you clear the merge conflict CC: @rithviknishad |
src/components/Medicine/MedicineAdministrationSheet/AdministrationTable.tsx
Outdated
Show resolved
Hide resolved
|
Caution Review failedThe pull request is closed. WalkthroughThe changes made in this pull request focus on the Changes
Assessment against linked issues
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
src/components/Medicine/MedicineAdministrationSheet/AdministrationTableRow.tsxOops! Something went wrong! :( ESLint: 8.57.1 Error: Failed to load parser '@typescript-eslint/parser' declared in '.eslintrc.json': Cannot find module '@typescript-eslint/parser'
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 (
|
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
🧹 Outside diff range and nitpick comments (1)
src/components/Medicine/MedicineAdministrationSheet/AdministrationTableRow.tsx (1)
62-80
: LGTM! The TableContent implementation effectively addresses the alignment issues.The component successfully implements:
- Center alignment using flex layout
- Text truncation for long content
- Responsive design with different layouts for mobile/desktop
- Proper handling of different dosage types
Consider these minor improvements:
- Extract the magic number for truncation width into a constant:
-<p className="max-w-[6rem] truncate"> +<p className="max-w-[var(--dosage-max-width)] truncate">
- Add type safety for the frequency translation key:
-t("PRESCRIPTION_FREQUENCY_" + prescription.frequency) +t(`PRESCRIPTION_FREQUENCY_${prescription.frequency as FrequencyType}`)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/components/Medicine/MedicineAdministrationSheet/AdministrationTable.tsx
(1 hunks)src/components/Medicine/MedicineAdministrationSheet/AdministrationTableRow.tsx
(2 hunks)
🔇 Additional comments (4)
src/components/Medicine/MedicineAdministrationSheet/AdministrationTable.tsx (2)
34-35
: LGTM! Simplified header structure.
The simplified structure with direct translation helps maintain consistent alignment.
37-44
:
Several improvements needed for the dosage header.
- The "Dosage &" text should be internationalized
- The mobile view hiding might affect the alignment objectives mentioned in the PR
- The current structure with separate
<p>
tags might contribute to alignment issues - Need to handle the case when prescriptions array is empty
Apply this diff to address the issues:
- <span className="hidden px-2 text-center text-xs leading-none lg:block">
- <p>Dosage &</p>
- <p>
- {prescriptions[0]?.dosage_type !== "PRN"
- ? t("frequency")
- : t("indicator")}
- </p>
- </span>
+ <span className="px-2 text-center text-xs leading-none">
+ <div className="flex flex-col items-center">
+ {t("dosage_and")}
+ {prescriptions.length > 0 && (
+ <span>
+ {prescriptions[0].dosage_type !== "PRN"
+ ? t("frequency")
+ : t("indicator")}
+ </span>
+ )}
+ </div>
+ </span>
Let's verify the mobile view behavior:
src/components/Medicine/MedicineAdministrationSheet/AdministrationTableRow.tsx (2)
238-245
: LGTM! Responsive integration maintains consistent alignment.
The dual rendering approach with proper visibility control ensures consistent alignment across viewports while maintaining the content structure.
238-245
: Verify mobile view layout implementation.
The current implementation appears to meet the mobile view requirements, but let's verify the complete structure.
✅ Verification successful
Mobile view implementation verified and correctly structured
The implementation follows responsive design best practices:
- Uses
block lg:hidden
for mobile-only content - Uses
hidden lg:table-cell
for desktop-only content - TableContent component is rendered appropriately in both views
- No conflicting layout directives found between mobile and desktop views
- Consistent with other responsive implementations in the codebase (as seen in multiple files using similar patterns)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify mobile layout implementation across related components
# Test 1: Check for mobile-specific layout classes
echo "Checking mobile layout implementation..."
rg -l "flex-col.*lg:flex-row|block.*lg:hidden|hidden.*lg:block" --type tsx
# Test 2: Verify consistent usage of breakpoints
echo "Verifying breakpoint consistency..."
rg "lg:[a-zA-Z-]+" --type tsx
# Test 3: Check for any conflicting layout directives
echo "Checking for layout conflicts..."
rg "flex.*(row|col).*lg:" --type tsx
Length of output: 478
Script:
#!/bin/bash
# Description: Verify mobile layout implementation across related components
# Test 1: Check for mobile-specific layout classes
echo "Checking mobile layout implementation..."
rg -l "flex-col.*lg:flex-row|block.*lg:hidden|hidden.*lg:block"
# Test 2: Verify consistent usage of breakpoints
echo "Verifying breakpoint consistency..."
rg "lg:[a-zA-Z-]+"
# Test 3: Check TableContent component usage
echo "Checking TableContent component usage..."
rg -A 5 "TableContent"
# Test 4: Check for any conflicting layout directives
echo "Checking for layout conflicts..."
rg "flex.*(row|col).*lg:"
Length of output: 33975
src/components/Medicine/MedicineAdministrationSheet/AdministrationTableRow.tsx
Outdated
Show resolved
Hide resolved
</th> | ||
<th> | ||
<span className="hidden px-2 text-center text-xs leading-none lg:block"> | ||
<p>Dosage &</p> |
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.
use translations for the text "Dosage"
src/components/Medicine/MedicineAdministrationSheet/AdministrationTableRow.tsx
Outdated
Show resolved
Hide resolved
src/components/Medicine/MedicineAdministrationSheet/AdministrationTableRow.tsx
Outdated
Show resolved
Hide resolved
@Sulochan-khadka Your efforts have helped advance digital healthcare and TeleICU systems. 🚀 Thank you for taking the time out to make CARE better. We hope you continue to innovate and contribute; your impact is immense! 🙌 |
Proposed Changes
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
New Features
DosageFrequencyInfo
component to enhance the display of dosage information based on prescription details.Bug Fixes