fix/INJIWEB-1789-isserspageupdate - #486
Conversation
Signed-off-by: sanehema9 <sanehema9@gmail.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 Possibly related PRs
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)
📝 Coding Plan
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 |
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 |
| 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.
| // 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
There was a problem hiding this comment.
The code changes are breaking the existing UI. Please ensure the existing UI is not impacted.
Signed-off-by: sanehema9 <sanehema9@gmail.com>
fixed the line break |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
inji-web/src/components/Common/HeaderTile.tsx (2)
7-7: Nit: Trailing space in className string.There's a trailing space at the end of the className on line 7. Consider removing it for consistency.
🧹 Remove trailing space
- className={"text-center font-bold text-iw-title mt-8 text-[24px] leading-[32px] "}>{content}</div> + className={"text-center font-bold text-iw-title mt-8 text-[24px] leading-[32px]"}>{content}</div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@inji-web/src/components/Common/HeaderTile.tsx` at line 7, The className string in HeaderTile's JSX contains a trailing space after the last class selector; remove the trailing space in the className prop on the element rendering {content} so the string ends with the final class (e.g., "leading-[32px]") without extra whitespace, ensuring the className value is normalized in the HeaderTile component.
8-9:whitespace-nowrapmay cause horizontal overflow on narrow screens.If
subContentis long, the text will not wrap and could overflow the container on mobile or smaller viewports. Consider adding overflow handling or allowing the text to wrap.💡 Option: Add overflow handling if nowrap is required
<div data-testid="HeaderTile-Text-SubContent" - className={"text-center text-iw-subTitle text-[14px] leading-[20px] font-medium mb-8 mt-2 whitespace-nowrap"}>{subContent}</div> + className={"text-center text-iw-subTitle text-[14px] leading-[20px] font-medium mb-8 mt-2 whitespace-nowrap overflow-hidden text-ellipsis"}>{subContent}</div>Alternatively, if wrapping is acceptable for long text, remove
whitespace-nowrapentirely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@inji-web/src/components/Common/HeaderTile.tsx` around lines 8 - 9, The HeaderTile component's subcontent div (data-testid="HeaderTile-Text-SubContent") uses whitespace-nowrap which will cause horizontal overflow for long subContent on small screens; either remove whitespace-nowrap to allow wrapping (so the text wraps naturally), or replace it with responsive/overflow-safe styling such as adding overflow-hidden, text-ellipsis/truncate and break-words (or make nowrap only on larger screens via sm:whitespace-nowrap) so long text is clipped/ellipsized instead of overflowing. Update the class on the HeaderTile subContent div accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@inji-web/src/components/Common/HeaderTile.tsx`:
- Line 7: The className string in HeaderTile's JSX contains a trailing space
after the last class selector; remove the trailing space in the className prop
on the element rendering {content} so the string ends with the final class
(e.g., "leading-[32px]") without extra whitespace, ensuring the className value
is normalized in the HeaderTile component.
- Around line 8-9: The HeaderTile component's subcontent div
(data-testid="HeaderTile-Text-SubContent") uses whitespace-nowrap which will
cause horizontal overflow for long subContent on small screens; either remove
whitespace-nowrap to allow wrapping (so the text wraps naturally), or replace it
with responsive/overflow-safe styling such as adding overflow-hidden,
text-ellipsis/truncate and break-words (or make nowrap only on larger screens
via sm:whitespace-nowrap) so long text is clipped/ellipsized instead of
overflowing. Update the class on the HeaderTile subContent div accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5c5dd193-4d10-4936-9e66-f6d216ffec29
📒 Files selected for processing (1)
inji-web/src/components/Common/HeaderTile.tsx
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
##Screenshot
Summary by CodeRabbit
Style
Chores
✏️ Tip: You can customize this high-level summary in your review settings.