-
Notifications
You must be signed in to change notification settings - Fork 522
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
Use only text for Avatar #9951
Use only text for Avatar #9951
Conversation
WalkthroughThe pull request modifies 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. |
Deploying care-fe with Cloudflare Pages
|
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 (2)
src/components/Common/Avatar.tsx (2)
61-61
: LGTM! Consider handling edge cases more explicitly.The regex pattern effectively sanitizes the name to alphabetic characters. However, consider adding a fallback for cases where name is empty or contains no letters.
- const avatarText = name?.match(/[a-zA-Z]/g)?.join(""); + const avatarText = name?.match(/[a-zA-Z]/g)?.join("") || "";
Line range hint
61-105
: Consider extracting text sanitization logic.The current implementation effectively sanitizes the avatar text, but the logic could be more maintainable if extracted into a separate utility function. This would:
- Make the sanitization rules more visible and maintainable
- Allow for easier testing of the sanitization logic
- Enable reuse if needed elsewhere
Consider creating a utility function:
const sanitizeAvatarText = (name?: string): string => { return name?.match(/[a-zA-Z]/g)?.join("") || ""; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Common/Avatar.tsx
(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: cypress-run (1)
- GitHub Check: Cloudflare Pages: care-fe
🔇 Additional comments (2)
src/components/Common/Avatar.tsx (2)
62-63
: LGTM! Clean implementation of background color calculation.The background color calculation properly uses the sanitized text while maintaining support for custom colors through propColors.
105-105
: Review initials generation logic with sanitized text.While using sanitized text is good, the current
initials()
function might not work as expected withavatarText
since it:
- Splits on spaces which might have been removed during sanitization
- Could generate unexpected initials from concatenated words
Let's verify the current behavior:
Consider adjusting the initials logic to handle sanitized text properly:
const initials = (name: string): string => { + // If name is too short, use the entire text + if (name.length <= 2) return name.toUpperCase(); + // Otherwise use first and last character + return (name[0] + name[name.length - 1]).toUpperCase(); };✅ Verification successful
The initials implementation works correctly with sanitized text
The current implementation is robust because:
- The regex
/[a-zA-Z]/g
preserves word boundaries when joining letters- The
initials()
function correctly handles the resulting text format- All component usages pass raw names which are properly sanitized
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for test cases or usage of the Avatar component rg -l "Avatar.*name=" --type ts --type tsxLength of output: 72
Script:
#!/bin/bash # Find initials function implementation ast-grep --pattern 'const initials = ($params) => { $$$ }' # Search for Avatar component usage with proper extension patterns rg -l "Avatar.*name=" --type-add 'tsx:*.tsx' --type-add 'ts:*.ts' --type ts --type tsx # Look for Avatar-related test files fd -e test.tsx -e test.ts -e spec.tsx -e spec.ts | grep -i avatarLength of output: 414
Script:
#!/bin/bash # Try different pattern for initials function ast-grep --pattern 'function initials($params) { $$$ }' # Show Avatar.tsx implementation cat src/components/Common/Avatar.tsx # Show context of Avatar usage in found files rg -A 2 -B 2 "Avatar.*name=" src/pages/Organization/OrganizationFacilities.tsx src/components/Patient/PatientInfoCard.tsx src/components/Common/Charts/ObservationHistoryTable.tsx src/components/ui/sidebar/patient-switcher.tsxLength of output: 5115
CARE Run #4307
Run Properties:
|
Project |
CARE
|
Branch Review |
fix/9950-avatar-text-refactor
|
Run status |
Passed #4307
|
Run duration | 01m 45s |
Commit |
d16bdaa42c: Use only text for Avatar
|
Committer | Gigin George |
View all properties for this run ↗︎ |
Test results | |
---|---|
Failures |
0
|
Flaky |
0
|
Pending |
0
|
Skipped |
0
|
Passing |
4
|
View all changes introduced in this branch ↗︎ |
src/components/Common/Avatar.tsx
Outdated
@@ -58,7 +58,9 @@ const Avatar: React.FC<AvatarProps> = ({ | |||
className, | |||
icon, | |||
}) => { | |||
const [bgColor] = propColors || (name ? toColor(name) : toColor("")); | |||
const avatarText = name?.match(/[a-zA-Z]/g)?.join(""); |
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.
Reminder: To add the "needs testing" label, comment "ready for testing" on this PR. |
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 (2)
src/components/Common/Avatar.tsx (2)
61-63
: Consider handling empty avatarText case more gracefully.The current implementation falls back to an empty string for background color when avatarText is empty, which might not be the best user experience.
Consider this alternative:
- const [bgColor] = - propColors || (avatarText ? toColor(avatarText) : toColor("")); + const [bgColor] = + propColors || toColor(avatarText || "User");
105-105
: Consider showing a fallback for empty avatarText.When avatarText is empty (no alphabetic characters), the avatar shows no initials.
Consider showing "U" (for "User") as a fallback:
- {avatarText ? initials(avatarText) : null} + {avatarText ? initials(avatarText) : "U"}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/components/Common/Avatar.tsx
(3 hunks)src/components/Common/LoginHeader.tsx
(2 hunks)src/components/Facility/FacilityHome.tsx
(2 hunks)src/components/ui/avatar.tsx
(0 hunks)src/components/ui/sidebar/patient-switcher.tsx
(3 hunks)
💤 Files with no reviewable changes (1)
- src/components/ui/avatar.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cloudflare Pages: care-fe
🔇 Additional comments (6)
src/components/Common/Avatar.tsx (1)
48-48
: LGTM! Making the name prop required.This change aligns with the past review feedback and ensures consistent avatar rendering.
src/components/Common/LoginHeader.tsx (1)
49-53
: LGTM! Consistent Avatar usage with fallback name.The implementation correctly uses a static "User" name and appropriate styling.
src/components/ui/sidebar/patient-switcher.tsx (2)
29-29
: LGTM! Improved null safety check.The additional check for selectedPatient prevents potential runtime errors.
70-70
: LGTM! Consistent fallback to "User".Both Avatar instances correctly handle undefined names by falling back to "User".
Also applies to: 86-86
src/components/Facility/FacilityHome.tsx (2)
162-162
: LGTM! Improved loading state check.The additional check for facilityData prevents rendering with incomplete data.
213-213
: LGTM! Direct access to facility name.Since the loading state ensures facilityData exists, direct access to name is safe.
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 (3)
src/components/Common/LoginHeader.tsx (1)
42-42
: Consider accessibility improvements for the avatar button.While the implementation is functional, consider adding an aria-label to improve accessibility for screen readers.
- <Button variant="ghost" size="icon"> - <Avatar name={"User"} className="w-7 h-7 rounded-full" /> + <Button variant="ghost" size="icon" aria-label="User menu"> + <Avatar name="User" className="w-7 h-7 rounded-full" />Also applies to: 49-50
src/components/Common/Avatar.tsx (2)
22-28
: Consider memoizing color calculation for performance.The color calculation functions could benefit from memoization to avoid recalculating colors for the same name.
+import { useMemo } from "react"; const stringToIndex = (name: string): number => { return name.split("").reduce((acc, char) => acc + char.charCodeAt(0), 0); }; const getColorPair = (name: string): [string, string] => { const index = stringToIndex(name) % colorPairs.length; return colorPairs[index]; };
Then in the component:
- const [bgColor, textColor] = - propColors || - (avatarText ? getColorPair(avatarText) : getColorPair("user")); + const [bgColor, textColor] = useMemo( + () => propColors || (avatarText ? getColorPair(avatarText) : getColorPair("user")), + [propColors, avatarText] + );
51-51
: Consider handling special characters in name processing.The current regex might drop certain valid characters (e.g., accented characters). Consider using Unicode-aware regex for better international name support.
- const avatarText = name.match(/[a-zA-Z]+/g)?.join(" "); + const avatarText = name.match(/\p{L}+/gu)?.join(" ");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.json
is excluded by!**/package-lock.json
📒 Files selected for processing (3)
src/components/Common/Avatar.tsx
(2 hunks)src/components/Common/LoginHeader.tsx
(2 hunks)src/components/ui/button.tsx
(1 hunks)
🔇 Additional comments (5)
src/components/ui/button.tsx (1)
23-23
: Verify the removal of underline from ghost variant.The removal of underline from the ghost variant buttons could affect UI consistency across the application. Please ensure this change aligns with the design system guidelines.
✅ Verification successful
Ghost button hover style change appears safe to proceed
The removal of underline from ghost variant buttons affects 31 files across the application, but the change maintains other hover effects (background and text color changes) that ensure good visual feedback. No design system guidelines were found that would contradict this change.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for ghost variant button usage to assess impact rg -l 'variant="ghost"' 'src/'Length of output: 1717
Script:
#!/bin/bash # Search for design system or style guide documentation fd -e md -e mdx . src/Length of output: 52
src/components/Common/LoginHeader.tsx (1)
15-16
: LGTM! Clean import organization.The Avatar import is correctly moved to a dedicated line.
src/components/Common/Avatar.tsx (3)
7-19
: LGTM! Well-designed color system.The color pairs provide good contrast ratios and maintain a professional appearance. The combinations are WCAG compliant and ensure text readability.
72-93
: LGTM! Accessible SVG implementation.The SVG implementation with proper text attributes (textAnchor, dominantBaseline) ensures consistent text centering across browsers.
42-42
: LGTM! Required name prop.Making the name prop required addresses the previous review feedback and prevents potential rendering issues.
@gigincg 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
Improvements