FIXED: Text overflows in the user management card details #9532 #9541
Conversation
…verlaps on mobile screens
WalkthroughThe pull request enhances the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (3)
src/components/Users/UserListAndCard.tsx (3)
8-13: Add a short docstring for the new imports.
These imports introduce new UI components for tooltips. Although the usage is clear, adding a short comment or docstring on top of this import block to explain their purpose can help future maintainers quickly identify where tooltips are sourced.
92-106: Create a reusable tooltip wrapper component to reduce duplication.
The tooltip structure for displaying the user’s name appears again for the username block. Consider extracting these repetitive sections into a small wrapper component that accepts the text as a prop, which will make the usage more concise and maintainable.
139-139: Optional: Provide accessibility text for the details button.
You might consider adding an aria-label to the button for better screen reader support, especially if the text is truncated on smaller screens.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Users/UserListAndCard.tsx(2 hunks)
🔇 Additional comments (1)
src/components/Users/UserListAndCard.tsx (1)
120-135: Ensure consistent styling and naming in the tooltip.
Currently, the approach is consistent with the user’s name tooltip. Please confirm that the styling (e.g., font size, whitespace, max-width) matches the user’s name tooltip to provide a uniform user experience. Additionally, consider i18n or a fallback string if the username is not available.
✅ Verification successful
Let me search for similar tooltip implementations and styling patterns in the codebase to verify the consistency.
Let me try one more search to find all tooltip implementations in the file to compare styling:
Styling is consistent with other tooltips in the component.
Based on the code review, the username tooltip implementation follows the same pattern as the user's name tooltip:
- Both use the same
TooltipProviderwithdelayDuration={200} - Both have the same
max-w-sm break-wordsstyling for the tooltip content - Both handle text overflow consistently with
text-ellipsis overflow-hidden whitespace-nowrap - Both are wrapped in the same container with responsive max-width constraints
- Fallback for username is handled via the optional chaining (
??) in the Avatar component
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for similar tooltip implementations
rg -U "TooltipProvider.*TooltipTrigger.*TooltipContent" -A 5
Length of output: 62
Script:
#!/bin/bash
# Search for username-related tooltips specifically
rg -U "username.*Tooltip" -A 5
Length of output: 32
Script:
#!/bin/bash
# Look for i18n implementations with usernames
rg "user\.username" -A 2 -B 2
Length of output: 5628
Script:
#!/bin/bash
# Search for any fallback handling with usernames
ast-grep --pattern 'user.username || $_'
Length of output: 42
Script:
#!/bin/bash
# Search for all tooltips in the UserListAndCard.tsx file
cat src/components/Users/UserListAndCard.tsx
Length of output: 14305
|
Okay, let me take care of it. |
|
@rithviknishad Fixed what you asked for |
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/components/Users/UserListAndCard.tsx (1)
Line range hint
293-307: Apply consistent overflow handling to list viewThe text overflow handling and tooltip implementation should be consistent across both card and list views.
Apply the same overflow handling and tooltip implementation to the name and username in the list view:
<div className="flex flex-col"> - <h1 id={`name-${user.username}`} className="text-sm font-medium"> - {formatName(user)} - </h1> - <span - id={`username-${user.username}`} - className="text-xs text-gray-500" - > - @{user.username} - </span> + <div className="min-w-0 max-w-[200px]"> + <TooltipProvider delayDuration={200}> + <Tooltip> + <TooltipTrigger asChild aria-label={`Full name: ${formatName(user)}`}> + <h1 id={`name-${user.username}`} className="text-sm font-medium text-ellipsis overflow-hidden whitespace-nowrap"> + {formatName(user)} + </h1> + </TooltipTrigger> + <TooltipContent role="tooltip"> + <p className="max-w-sm break-words">{formatName(user)}</p> + </TooltipContent> + </Tooltip> + </TooltipProvider> + </div> + <div className="min-w-0 max-w-[200px]"> + <TooltipProvider delayDuration={200}> + <Tooltip> + <TooltipTrigger asChild aria-label={`Username: ${user.username}`}> + <span + id={`username-${user.username}`} + className="text-xs text-gray-500 block text-ellipsis overflow-hidden whitespace-nowrap" + > + @{user.username} + </span> + </TooltipTrigger> + <TooltipContent role="tooltip"> + <p className="max-w-sm break-words">@{user.username}</p> + </TooltipContent> + </Tooltip> + </TooltipProvider> + </div> </div>
🧹 Nitpick comments (1)
src/components/Users/UserListAndCard.tsx (1)
91-106: Enhance tooltip accessibility while maintaining the overflow fixThe implementation effectively addresses text overflow using responsive max-width constraints and proper text truncation. However, let's enhance the tooltip accessibility.
Consider these accessibility improvements:
<TooltipProvider delayDuration={200}> <Tooltip> - <TooltipTrigger asChild> + <TooltipTrigger asChild aria-label={`Full name: ${formatName(user)}`}> <h1 id={`name-${user.username}`} className="text-base font-bold text-ellipsis overflow-hidden whitespace-nowrap" > {formatName(user)} </h1> </TooltipTrigger> - <TooltipContent> + <TooltipContent role="tooltip"> <p className="max-w-sm break-words">{formatName(user)}</p> </TooltipContent> </Tooltip> </TooltipProvider>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Users/UserListAndCard.tsx(2 hunks)
🔇 Additional comments (1)
src/components/Users/UserListAndCard.tsx (1)
8-13: LGTM: Clean tooltip component imports
The tooltip-related imports are properly organized and follow the project's import structure.
| <div className="min-w-0 max-w-full sm:max-w-[200px] md:max-w-[300px] lg:max-w-[400px]"> | ||
| <span | ||
| className="text-sm text-gray-500 block text-ellipsis overflow-hidden whitespace-nowrap" | ||
| id={`username-${user.username}`} | ||
| > | ||
| {user.username} | ||
| </span> | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add tooltip for username consistency
While the username display properly handles overflow, it should have the same tooltip functionality as the name display for consistency.
Add tooltip wrapper for username:
<div className="min-w-0 max-w-full sm:max-w-[200px] md:max-w-[300px] lg:max-w-[400px]">
+ <TooltipProvider delayDuration={200}>
+ <Tooltip>
+ <TooltipTrigger asChild aria-label={`Username: ${user.username}`}>
<span
className="text-sm text-gray-500 block text-ellipsis overflow-hidden whitespace-nowrap"
id={`username-${user.username}`}
>
{user.username}
</span>
+ </TooltipTrigger>
+ <TooltipContent role="tooltip">
+ <p className="max-w-sm break-words">{user.username}</p>
+ </TooltipContent>
+ </Tooltip>
+ </TooltipProvider>
</div>Committable suggestion skipped: line range outside the PR's diff.
| <div className="min-w-0 max-w-full sm:max-w-[200px] md:max-w-[300px] lg:max-w-[400px]"> | ||
| <span | ||
| className="text-sm text-gray-500 block text-ellipsis overflow-hidden whitespace-nowrap" | ||
| id={`username-${user.username}`} | ||
| > | ||
| {user.username} | ||
| </span> | ||
| </div> |
There was a problem hiding this comment.
do revert all changes to the username. there are still some changes
There was a problem hiding this comment.
You mean I should revert all changes to username means username inside that tooltip or the original code?
There was a problem hiding this comment.
Okay, I will just use original code and add tooltip to name only as other maintainer suggested
rithviknishad
added
changes required
and removed
question
|
@rithviknishad created PR of what you asked for |
rithviknishad
added
needs testing
needs review
and removed
question
| @@ -93,7 +93,7 @@ const getNameAndStatusCard = ( | |||
| <TooltipTrigger asChild> | |||
| <h1 | |||
| id={`name-${user.username}`} | |||
| className="text-base font-bold" | |||
| className="text-base font-bold truncate max-w-[200px]" | |||
There was a problem hiding this comment.
use rem values over pixels. pixels can behave differently on different DPIs
|
|
@rithviknishad if there is no extra changes required !!Can you merge the PR? |
rithviknishad
added
needs review
and removed
question
| {formatName(user)} | ||
| </h1> | ||
| </TooltipTrigger> | ||
| <TooltipContent> |
There was a problem hiding this comment.
I'd suggest a bit more styling to the tooltip content. Check the tooltip for Occupancy Badge in Facility for instance:
There was a problem hiding this comment.
Okay, I just added more styling and dynamic transitions on state and side. @Jacobjeevan
Co-authored-by: Jacob John Jeevan <[email protected]>
Proposed Changes
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
New Features
Style