-
-
Notifications
You must be signed in to change notification settings - Fork 49
fix(ui): align Repository & Self-Host typography and layout with Dashboard #477
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
base: master
Are you sure you want to change the base?
Conversation
WalkthroughIntroduces muted/small typography components into self-host repository list, repository cards, and page header layout; moves a description block below the header and adjusts header container classes. Removes the GithubRepositoriesSkeletonLoader export and its implementation. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
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: 1
♻️ Duplicate comments (1)
view/app/self-host/page.tsx (1)
101-102
: Verify semantic HTML structure (same concern as list-repositories.tsx).Similar to the concern raised in
list-repositories.tsx
(line 69), passing aTypographyH1
component to thelabel
prop could create nested heading elements or multipleh1
tags on the page. Please refer to the verification script in the previous file's review.
🧹 Nitpick comments (1)
view/app/self-host/components/github-repositories/list-repositories.tsx (1)
34-34
: Redundant condition check.The
&& !isLoading
check is redundant since this code executes in the else block after theif (isLoading)
check on line 30, meaning!isLoading
is always true here.Apply this diff to simplify:
- if (paginatedApplications?.length === 0 && !isLoading) { + if (paginatedApplications?.length === 0) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
view/app/self-host/components/github-repositories/list-repositories.tsx
(3 hunks)view/app/self-host/components/github-repositories/repository-card.tsx
(5 hunks)view/app/self-host/page.tsx
(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
view/app/self-host/page.tsx (1)
view/components/ui/typography.tsx (1)
TypographyH1
(1-7)
view/app/self-host/components/github-repositories/list-repositories.tsx (1)
view/components/ui/typography.tsx (1)
TypographyH1
(1-7)
view/app/self-host/components/github-repositories/repository-card.tsx (1)
view/components/ui/typography.tsx (2)
TypographySmall
(61-71)TypographyMuted
(73-77)
🔇 Additional comments (8)
view/app/self-host/components/github-repositories/list-repositories.tsx (1)
9-10
: LGTM! Typography import aligns with dashboard consistency.The import and comment clearly document the intent to match dashboard typography.
view/app/self-host/page.tsx (2)
19-20
: LGTM! Typography imports consolidated.Consolidating the typography imports improves readability and maintainability.
83-85
: LGTM! Consistent typography usage.The use of
TypographyH2
andTypographyMuted
maintains consistency with the dashboard design system.view/app/self-host/components/github-repositories/repository-card.tsx (5)
9-10
: LGTM! Typography imports added.The import statement and comment clearly indicate the use of dashboard typography components.
29-29
: LGTM! Cleaned up double space in className.Removing the extra space between hover classes improves code consistency.
33-38
: LGTM! Typography and sizing updates align with dashboard design.The changes correctly implement:
- Reduced icon size (20px) to match smaller typography
- Responsive text sizing (
text-sm sm:text-base
)TypographySmall
wrapper for consistent stylingThese changes align well with the PR objectives to match dashboard card design.
52-55
: LGTM! Description wrapped with TypographyMuted.The
TypographyMuted
wrapper provides consistent styling for secondary text across the application.
124-124
: LGTM! Skeleton loader updated to match component structure.The skeleton CardTitle classes now match the updated component sizing (
text-sm sm:text-base
), ensuring visual consistency during loading states.
view/app/self-host/components/github-repositories/list-repositories.tsx
Outdated
Show resolved
Hide resolved
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
view/app/self-host/page.tsx (1)
92-117
: Refactor to use JSX children instead of children prop.The header changes correctly prevent nested headings and the description block aligns with the Dashboard pattern. However, the
children
prop usage at line 101 should be refactored to use JSX children syntax instead.As per static analysis guidelines, apply this diff to use JSX children:
<DahboardUtilityHeader<Application> searchTerm={searchTerm} handleSearchChange={handleSearchChange} sortConfig={sortConfig} onSortChange={onSortChange} sortOptions={sortOptions} /* (changed) pass plain string, no nested heading */ label={t('selfHost.page.title')} className="mt-5 mb-2 flex flex-col sm:flex-row sm:justify-between sm:items-center gap-2" - children={ - <AnyPermissionGuard permissions={['deploy:create']} loadingFallback={null}> - <Button - className="mb-4 w-max flex justify-self-end mt-4" - onClick={() => { - router.push('/self-host/create'); - }} - > - {t('selfHost.page.createButton')} - </Button> - </AnyPermissionGuard> - } - /> + > + <AnyPermissionGuard permissions={['deploy:create']} loadingFallback={null}> + <Button + className="mb-4 w-max flex justify-self-end mt-4" + onClick={() => { + router.push('/self-host/create'); + }} + > + {t('selfHost.page.createButton')} + </Button> + </AnyPermissionGuard> + </DahboardUtilityHeader> {/* (added) description outside header */} <TypographyMuted className="mb-5"> {t('selfHost.page.description')}view/app/self-host/components/github-repositories/repository-card.tsx (1)
5-5
: Remove or re-exportGithubRepositoriesSkeletonLoader
.
Inlist-repositories.tsx
(line 5) you importGithubRepositoriesSkeletonLoader
fromrepository-card.tsx
, but that file no longer exports it—this will break the build. Either remove the import and its usage (line 31) or re-introduce and export the loader component inrepository-card.tsx
.
🧹 Nitpick comments (2)
view/app/self-host/components/github-repositories/list-repositories.tsx (1)
34-40
: Simplify the empty state wrapper.The
div
wrapper withtext-center
class may be redundant if the parent container already handles centering. Consider removing the wrapper unless explicit centering is required at this level.Apply this diff to simplify:
- if (paginatedApplications?.length === 0 && !isLoading) { - return ( - <div className="text-center"> - {t('selfHost.repositories.noRepositories')} - </div> - ); - } + if (paginatedApplications?.length === 0 && !isLoading) { + return t('selfHost.repositories.noRepositories'); + }view/app/self-host/components/github-repositories/repository-card.tsx (1)
33-38
: Simplify to avoid redundant typography styling.The CardTitle already applies responsive text sizing (
text-xs sm:text-sm font-bold
), making the nestedTypographySmall
wrapper redundant.TypographySmall
applies its owntext-sm
andfont-medium
classes, which may conflict with the CardTitle's explicit styling.Apply this diff to simplify:
- <CardTitle className="flex items-center gap-2 text-xs sm:text-sm font-bold"> + <CardTitle className="flex items-center gap-2"> <Github className="text-primary" size={20} /> <TypographySmall> {name || t('selfHost.repositoryCard.unnamed')} </TypographySmall>Or, if you want to keep the custom sizing on CardTitle, remove TypographySmall:
<CardTitle className="flex items-center gap-2 text-xs sm:text-sm font-bold"> <Github className="text-primary" size={20} /> - <TypographySmall> - {name || t('selfHost.repositoryCard.unnamed')} - </TypographySmall> + {name || t('selfHost.repositoryCard.unnamed')}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
view/app/self-host/components/github-repositories/list-repositories.tsx
(3 hunks)view/app/self-host/components/github-repositories/repository-card.tsx
(3 hunks)view/app/self-host/page.tsx
(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
view/app/self-host/components/github-repositories/list-repositories.tsx (1)
view/components/ui/typography.tsx (1)
TypographyMuted
(73-77)
view/app/self-host/components/github-repositories/repository-card.tsx (1)
view/components/ui/typography.tsx (2)
TypographySmall
(61-71)TypographyMuted
(73-77)
view/app/self-host/page.tsx (2)
view/components/rbac/PermissionGuard.tsx (2)
AnyPermissionGuard
(42-58)ResourceGuard
(78-95)view/components/ui/typography.tsx (1)
TypographyMuted
(73-77)
🪛 Biome (2.1.2)
view/app/self-host/page.tsx
[error] 101-101: Avoid passing children using a prop
The canonical way to pass children in React is to use JSX elements
(lint/correctness/noChildrenProp)
🔇 Additional comments (6)
view/app/self-host/components/github-repositories/list-repositories.tsx (2)
9-10
: LGTM!The TypographyMuted import is necessary for the new description block added below.
72-79
: Past issue resolved; layout and description align with Dashboard pattern.The changes correctly address the nested heading issue from the previous review by passing a plain string to the
label
prop instead of aTypographyH1
component. The responsive flex layout and new description block maintain consistency with the Dashboard design.view/app/self-host/page.tsx (2)
19-20
: LGTM!Consolidating the typography imports and adding TypographyMuted supports the new description blocks added throughout the file.
78-83
: LGTM!The access denied message correctly uses TypographyH2 and TypographyMuted, maintaining consistency with the updated typography pattern across the application.
view/app/self-host/components/github-repositories/repository-card.tsx (2)
9-10
: LGTM!The typography imports are necessary for the title and description changes that follow.
52-55
: LGTM!Wrapping the description with TypographyMuted inside CardDescription correctly applies the muted styling while preserving the line-clamp behavior. This aligns with the consistent typography pattern across the application.
Changes Made
Self-Host (page.tsx):
Updated header to use TypographyH1 for title and TypographyMuted for description.
Unified layout spacing with flex flex-col sm:flex-row sm:justify-between sm:items-center gap-2.
Access denied message uses TypographyH2 + TypographyMuted for consistency.
Repositories List (list-repositories.tsx):
Header updated to TypographyH1 + TypographyMuted.
Layout aligned with Dashboard Utility Header.
Repository Card (repository-card.tsx):
Repository name updated to TypographySmall with text-xs sm:text-sm font-bold.
Description uses TypographyMuted for consistent style.
Alignment of badges, icons, and metadata (stars, forks, issues) unified with Dashboard card design.
Summary by CodeRabbit
Style
Documentation