Conversation
Signed-off-by: sanehema9 <sanehema9@gmail.com>
WalkthroughUI/layout-only updates: responsive grid wrappers and minor class/tags adjustments across credential and issuer components and pages; Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
inji-web/src/components/Credentials/CredentialList.tsx (1)
97-119: Layout inconsistency between success and empty/error states.The success case returns a React Fragment, making
HeaderTileand the credentials container separate grid items within the parent grid (grid-cols-1 sm:grid-cols-2 lg:grid-cols-3). This meansHeaderTilewill only occupy 1 column on larger screens.However, the empty/error states (lines 76-84) wrap both
HeaderTileand content in a single<div>, making them one grid item.Consider wrapping both elements in a single container with
col-span-fullfor consistent layout:Suggested fix for consistent layout
return ( - <> + <div className="col-span-full"> <HeaderTile content={t("containerHeading")} subContent={t("containerSubHeading")} /> <div className="col-span-full flex flex-wrap gap-3 p-4 pb-20 justify-start"> + <div className="flex flex-wrap gap-3 p-4 pb-20 justify-start"> {filteredCredentialsWithLangSupport.map( ... )} </div> - </> + </div> );
🤖 Fix all issues with AI agents
In @inji-web/src/components/Issuers/IssuersList.tsx:
- Around line 32-34: The Issuer component is missing the required index prop
when rendered in the map; update the issuers.filtered_issuers.map callback in
IssuersList (where <Issuer key={issuer.issuer_id} issuer={issuer} /> is used) to
accept the map index and pass it as the index prop to <Issuer> (matching
IssuerProps and the index expected by ItemBox), e.g., map((issuer, index) =>
<Issuer key={issuer.issuer_id} issuer={issuer} index={index} />).
🧹 Nitpick comments (3)
inji-web/src/components/Issuers/IssuersList.tsx (1)
18-37: Inconsistent indentation throughout the component.The file mixes 2-space and 4-space indentation (e.g., lines 22-26 use 2-space, while lines 30, 35-36 use 4-space). Consider running a formatter to ensure consistent code style.
inji-web/src/pages/IssuersPage.tsx (2)
50-62: Consider simplifying error handling logic.The
try/catchininitializeIssuersDatacatches errors from bothfetchUserProfileandfetchIssuers. However,fetchIssuersalready handles its own errors internally (lines 29-31) and won't throw. The outercatchblock will only catch errors fromfetchUserProfile.For clarity, consider either:
- Wrapping only
fetchUserProfilein try/catch, or- Adding a comment explaining the intended error handling scope.
♻️ Suggested clarification
const initializeIssuersData = async () => { if (isUserLoggedIn()) { try { await fetchUserProfile(); - await fetchIssuers(); } catch (error: any) { console.error("Error fetching user profile:", error); toast.error(t("errorContent")); + return; } - } else { - await fetchIssuers(); } + await fetchIssuers(); };
68-78: Inconsistent indentation in JSX.Similar to
IssuersList.tsx, this file mixes 2-space (lines 70-74) and 4-space (lines 72-76) indentation. Consider running a formatter for consistent styling across the codebase.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
inji-web/src/components/Credentials/CredentialList.tsxinji-web/src/components/Credentials/CredentialListWrapper.tsxinji-web/src/components/Issuers/IssuersList.tsxinji-web/src/pages/CredentialsPage.tsxinji-web/src/pages/IssuersPage.tsx
🧰 Additional context used
🧬 Code graph analysis (3)
inji-web/src/components/Issuers/IssuersList.tsx (6)
inji-web/src/components/Common/GradientWrapper.tsx (1)
GradientWrapper(4-25)inji-web/src/components/Common/SpinningLoader.tsx (1)
SpinningLoader(4-21)inji-web/src/components/Common/HeaderTile.tsx (1)
HeaderTile(4-11)inji-web/src/components/Common/EmptyListContainer.tsx (1)
EmptyListContainer(4-13)inji-web/src/types/data.d.ts (1)
IssuerObject(42-57)inji-web/src/components/Issuers/Issuer.tsx (1)
Issuer(12-30)
inji-web/src/pages/CredentialsPage.tsx (3)
inji-web/src/types/redux.d.ts (1)
RootState(3-3)inji-web/src/types/data.d.ts (2)
IssuerWellknownDisplayArrayObject(7-14)IssuerObject(42-57)inji-web/src/components/Credentials/CredentialListWrapper.tsx (1)
CredentialListWrapper(10-22)
inji-web/src/components/Credentials/CredentialListWrapper.tsx (1)
inji-web/src/components/Credentials/CredentialList.tsx (1)
CredentialList(18-121)
🔇 Additional comments (4)
inji-web/src/pages/CredentialsPage.tsx (1)
82-87: Good responsive container structure.The container wrapper with responsive padding (
px-4 sm:px-6 md:px-10 lg:px-20) provides proper mobile-first responsive design, while thecontainer mx-autoensures consistent max-width and centering.inji-web/src/components/Credentials/CredentialListWrapper.tsx (2)
5-10: Good use of optional prop with default value.Making
classNameoptional with an empty string default is a clean approach that maintains backward compatibility.
16-19: Grid wrapper may not achieve intended responsive layout.The responsive grid (
grid-cols-1 sm:grid-cols-2 lg:grid-cols-3) is applied here, butCredentialListreturns a Fragment containing:
HeaderTile(becomes 1 grid item)- A div with
flex flex-wrapcontaining credential cardsThe credential cards themselves use flexbox layout (
flex flex-wrap), not CSS grid, so this grid wrapper won't directly control the card column layout. The grid only affects the Fragment's direct children (HeaderTileand the wrapper div), not the individual cards.If the goal is responsive card columns, consider either:
- Moving the grid classes to the credential cards' container inside
CredentialList- Or restructuring so credential cards are direct grid children
Please verify the rendered layout matches expectations across breakpoints (mobile, tablet, desktop).
inji-web/src/pages/IssuersPage.tsx (1)
15-15: LGTM on simplified component signature.Removing the unused
classNameprop and simplifying the type toReact.FCis a good cleanup.
Signed-off-by: sanehema9 <sanehema9@gmail.com>
Fixed the missing index prop in IssuersList.tsx. The latest commit addresses the issue flagged by the bot. Ready for review |
inji-web/src/pages/IssuersPage.tsx
Outdated
| import {useUser} from "../hooks/User/useUser"; | ||
|
|
||
| export const IssuersPage: React.FC<IssuerPageProps> = ({className}) => { | ||
| export const IssuersPage: React.FC = () => { |
There was a problem hiding this comment.
Did we confirm on the usages of the components before removing the className ? Please check the post login flow if the alignment is working properly on it. Please add the video for it in the PR post checking it.
inji-web/src/pages/IssuersPage.tsx
Outdated
| // Excludes issuers with protocol 'OTP' and those whose issuer_id is in the ignoredIssuersFromRendering list (or contains any ignored issuer id as a substring). | ||
| issuer.protocol !== 'OTP' && | ||
| (ignoredIssuerIds.length === 0 || !ignoredIssuerIds.some((ignoredIssuerId) => | ||
| issuer.protocol !== "OTP" && |
There was a problem hiding this comment.
Did we intend to remove the comment here ?
Signed-off-by: sanehema9 <sanehema9@gmail.com>
Signed-off-by: sanehema9 <sanehema9@gmail.com>
|
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
inji-web/src/pages/IssuersPage.tsx (1)
70-76:⚠️ Potential issue | 🟠 MajorAdd top margin to IssuersList to maintain consistent spacing.
The flex wrapper (line 71) with
gap-6provides 1.5rem spacing betweenIntroBoxandSearchIssuer, butIssuersList(line 75) is a sibling outside this wrapper without its own top margin. This creates inconsistent vertical spacing. Addmt-6to theIssuersListcomponent to match the spacing pattern, or move it inside the flex wrapper.
Rudhhi-Shah-14
left a comment
Rudhhi-Shah-14
left a comment
There was a problem hiding this comment.
The code changes are breaking the existing UI. Please ensure the existing UI is not impacted.
3 participants
Description
changed the files
CredentialList.tsx , CredentialListWrapper.tsx , CredentialsPage.tsx , Issuerspage.tsx and IssuersList.tsx
Issue ticket number and link
INJIWEB-1789(https://mosip.atlassian.net/browse/INJIWEB-1789)
Video
INJIWEB-1789.mp4
Summary by CodeRabbit
Style
Chores
✏️ Tip: You can customize this high-level summary in your review settings.