Update the Card Design in Notice Board #9343
Conversation
WalkthroughThe 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: 1
🧹 Outside diff range and nitpick comments (1)
src/components/Notifications/NoticeBoard.tsx (1)
26-26: Consider adjusting card margin for consistencyWhile the card styling looks good, the
my-1margin seems inconsistent with thegap-6used in the grid. Consider removing the margin since the grid gap already provides sufficient spacing.-className="overflow-hidden rounded shadow-md my-1" +className="overflow-hidden rounded shadow-md"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/components/Notifications/NoticeBoard.tsx(1 hunks)
🔇 Additional comments (2)
src/components/Notifications/NoticeBoard.tsx (2)
22-22: LGTM! Responsive grid layout with proper spacing
The grid container implementation with responsive columns and consistent gaps provides a clean layout across different screen sizes.
36-44: Consider using a more appropriate icon for user attribution
The Facebook Messenger icon (l-facebook-messenger) seems out of place for representing the user who caused the notification. Consider using a more appropriate icon like l-user or l-user-circle.
<div className="text-md my-1 text-secondary-700 mx-2 px-3">
- <CareIcon icon="l-facebook-messenger" className="mr-2" />
+ <CareIcon icon="l-user" className="mr-2" />
{formatName(item.caused_by)} -{" "}Let's verify that all text strings are properly internationalized:
|
Closing as duplicate of #9342 Please update the original PR instead 👍 |
|
@Jacobjeevan i closed the original PR becuase many unwanted changes got commited into that . Try to open these and get reviewd |
|
But both PRs are using the same branch @AnveshNalimela. So it's equivalent to re-opening your old PR right? |
|
@rithviknishad i just seen the in that PR 5 files got changed which are irrelavent and lot of commits so i closed that and reverted all changes and made the new PR |
|
@Jacobjeevan reopening this as github doesn't allow reopening old PR's if there are force pushes on to the same branch. |
There was a problem hiding this comment.
by avatar in my previous change request, I meant this: https://github.com/ohcnetwork/care_fe/blob/develop/src/components/Common/Avatar.tsx
Not an icon.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
src/components/Notifications/NoticeBoard.tsx (2)
12-12: Consider using alias import path for consistencyThe Avatar import uses a relative path while other component imports use the
@/componentsalias. Consider updating for consistency:-import { Avatar } from "../Common/Avatar"; +import { Avatar } from "@/components/Common/Avatar";
Line range hint
76-76: Add missing translations for internationalization complianceThe PR objectives mention I18n compliance. The text "No Notice Available" should use the translation function:
- No Notice Available + {t("no_notice_available")}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Notifications/NoticeBoard.tsx(2 hunks)
🔇 Additional comments (2)
src/components/Notifications/NoticeBoard.tsx (2)
24-28: LGTM! Improved card layout and responsiveness
The grid layout with responsive columns and visual enhancements (shadow, rounded corners) improves the overall design.
30-39: Reconsider the messenger icon choice
The l-facebook-messenger icon might not be the most appropriate choice for system notifications. Consider using a more generic notification or system message icon like l-bell or l-envelope.
Previous accessibility and readability concerns remain unaddressed
As mentioned in the previous review:
- Add an aria-label for accessibility
- Reconsider using
font-monofor better readability of longer messages
| <div className="bg-gray-200 py-2 flex items-center "> | ||
| <Avatar | ||
| name={item.caused_by.user_type || ""} | ||
| className="border-0 border-b border-b-secondary-300 rounded-full h-10 w-10 ml-5" | ||
| /> | ||
| <div className="text-md my-1 text-secondary-700 px-3"> | ||
| <div className="flex items-center"> | ||
| {formatName(item.caused_by)} -{" "} | ||
| <span className="font-bold text-primary-700 ml-1"> | ||
| {item.caused_by.user_type} | ||
| </span> | ||
| </div> | ||
| <div className="text-xs text-secondary-900 font-medium "> | ||
| {t("on")}: {formatDateTime(item.created_date)} | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance accessibility and reduce redundancy
The user info section has two areas of improvement:
- The Avatar component should have an alt text or aria-label for accessibility
- The user type is displayed twice (in Avatar and text), which is redundant
<Avatar
name={item.caused_by.user_type || ""}
+ aria-label={`${formatName(item.caused_by)}'s avatar`}
className="border-0 border-b border-b-secondary-300 rounded-full h-10 w-10 ml-5"
/>
<div className="text-md my-1 text-secondary-700 px-3">
<div className="flex items-center">
- {formatName(item.caused_by)} -{" "}
- <span className="font-bold text-primary-700 ml-1">
- {item.caused_by.user_type}
- </span>
+ {formatName(item.caused_by)}
</div>📝 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.
| <div className="bg-gray-200 py-2 flex items-center "> | |
| <Avatar | |
| name={item.caused_by.user_type || ""} | |
| className="border-0 border-b border-b-secondary-300 rounded-full h-10 w-10 ml-5" | |
| /> | |
| <div className="text-md my-1 text-secondary-700 px-3"> | |
| <div className="flex items-center"> | |
| {formatName(item.caused_by)} -{" "} | |
| <span className="font-bold text-primary-700 ml-1"> | |
| {item.caused_by.user_type} | |
| </span> | |
| </div> | |
| <div className="text-xs text-secondary-900 font-medium "> | |
| {t("on")}: {formatDateTime(item.created_date)} | |
| </div> | |
| <div className="bg-gray-200 py-2 flex items-center "> | |
| <Avatar | |
| name={item.caused_by.user_type || ""} | |
| aria-label={`${formatName(item.caused_by)}'s avatar`} | |
| className="border-0 border-b border-b-secondary-300 rounded-full h-10 w-10 ml-5" | |
| /> | |
| <div className="text-md my-1 text-secondary-700 px-3"> | |
| <div className="flex items-center"> | |
| {formatName(item.caused_by)} | |
| </div> | |
| <div className="text-xs text-secondary-900 font-medium "> | |
| {t("on")}: {formatDateTime(item.created_date)} | |
| </div> |
Proposed Changes
Before
After:
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
New Features
Bug Fixes