WEB-378-improve the css office-navigation-page #2724
Conversation
WalkthroughMoved office externalId rendering from the header into card content and replaced it with the Changes
Sequence Diagram(s)(omitted — changes are UI/template and styling only; no control-flow or new interactions to visualize) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/navigation/office-navigation/office-navigation.component.ts (1)
3-21: Critical:MatCardContentimported but not included in theimportsarray.Line 3 imports
MatCardContentfrom@angular/material/card, but it's not listed in the component'simportsarray (lines 13-21). Since the template uses<mat-card-content>, this will cause a runtime error in standalone components.Apply this diff to add
MatCardContentto the imports array:imports: [ ...STANDALONE_SHARED_IMPORTS, MatCardHeader, FaIconComponent, MatCardTitleGroup, MatCardTitle, + MatCardContent, ExternalIdentifierComponent, DateFormatPipe ]
🧹 Nitpick comments (2)
src/app/navigation/office-navigation/office-navigation.component.ts (1)
24-25: Use strict typing instead ofanyfor Input properties.The
@Input()properties useanytype, which bypasses TypeScript's type safety and violates Angular best practices. Define proper interfaces forofficeDataandemployeeDatato enable compile-time type checking and improve maintainability.As per coding guidelines.
src/app/navigation/office-navigation/office-navigation.component.html (1)
16-22: Avoid duplicate conditional checks for the same condition.Both the label (lines 16-18) and value (lines 20-22) blocks check
officeData.externalIdindependently, causing Angular to evaluate the condition twice. This is inefficient and violates DRY principles.Apply this diff to wrap both blocks in a single conditional container:
- <div class="flex-50 mat-body-strong" *ngIf="officeData.externalId"> - {{ 'labels.inputs.External Id' | translate }} - </div> - - <div class="flex-50" *ngIf="officeData.externalId"> - <mifosx-external-identifier externalId="{{ officeData.externalId }}"></mifosx-external-identifier> - </div> + <ng-container *ngIf="officeData.externalId"> + <div class="flex-50 mat-body-strong"> + {{ 'labels.inputs.External Id' | translate }} + </div> + + <div class="flex-50"> + <mifosx-external-identifier externalId="{{ officeData.externalId }}"></mifosx-external-identifier> + </div> + </ng-container>
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/app/navigation/navigation.component.scss(1 hunks)src/app/navigation/office-navigation/office-navigation.component.html(2 hunks)src/app/navigation/office-navigation/office-navigation.component.scss(1 hunks)src/app/navigation/office-navigation/office-navigation.component.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/app/**
⚙️ CodeRabbit configuration file
src/app/**: For Angular code: verify component separation, trackBy on *ngFor,
strict type safety, and clean observable patterns.
Files:
src/app/navigation/office-navigation/office-navigation.component.scsssrc/app/navigation/navigation.component.scsssrc/app/navigation/office-navigation/office-navigation.component.htmlsrc/app/navigation/office-navigation/office-navigation.component.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run Lint, Build and Deploy
🔇 Additional comments (2)
src/app/navigation/navigation.component.scss (1)
1-57: LGTM! Well-structured responsive layout.The styling introduces a clean, maintainable structure with:
- Proper component encapsulation using
:host- CSS Grid for responsive content layout
- Modern CSS range syntax for media queries
- Appropriate mobile-first breakpoints
The layout complements the child component's styling without creating conflicts.
src/app/navigation/office-navigation/office-navigation.component.scss (1)
1-121: LGTM! Well-architected CSS with modern patterns.The refactored styling demonstrates excellent practices:
- CSS custom properties enable clean theming and dark mode support
- Semantic variable naming improves maintainability
- Responsive design with appropriate breakpoints
- Smooth transitions and hover effects enhance UX
- Proper use of CSS Grid for layout
| mat-card-subtitle { | ||
| p { | ||
| color: var(--header-subtitle-color, #000); | ||
| margin: 0; | ||
| font-size: 0.875rem; | ||
| display: flex; | ||
| align-items: center; | ||
| gap: 0.5rem; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove unused mat-card-subtitle styling.
The component no longer uses MatCardSubtitle (removed from imports in the TypeScript file), and the HTML template doesn't contain <mat-card-subtitle> elements. This styling block is dead code.
Apply this diff to remove the unused styling:
mat-card-title h2 {
font-weight: 500;
font-size: 1.5rem;
color: var(--header-title-color, #000);
margin: 0;
line-height: 1.4;
}
-
- mat-card-subtitle {
- p {
- color: var(--header-subtitle-color, #000);
- margin: 0;
- font-size: 0.875rem;
- display: flex;
- align-items: center;
- gap: 0.5rem;
- }
- }
}📝 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.
| mat-card-subtitle { | |
| p { | |
| color: var(--header-subtitle-color, #000); | |
| margin: 0; | |
| font-size: 0.875rem; | |
| display: flex; | |
| align-items: center; | |
| gap: 0.5rem; | |
| } | |
| } | |
| mat-card-title h2 { | |
| font-weight: 500; | |
| font-size: 1.5rem; | |
| color: var(--header-title-color, #000); | |
| margin: 0; | |
| line-height: 1.4; | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/app/navigation/office-navigation/office-navigation.component.scss around
lines 40 to 49, remove the unused mat-card-subtitle styling block since the
component no longer imports or uses MatCardSubtitle and the template has no
<mat-card-subtitle> elements; delete the entire mat-card-subtitle { p { ... } }
block to eliminate dead CSS.
| &:last-child, | ||
| &:nth-last-child(2) { | ||
| border-bottom: none; | ||
| } |
There was a problem hiding this comment.
Border removal logic may fail with conditional rendering.
The logic removes borders from the last two child elements, assuming they always appear in pairs. However, when officeData.externalId or officeData.parentName are absent, the grid will have fewer elements, and the border removal may apply to incorrect elements, creating visual inconsistencies.
Consider using a more robust approach that doesn't rely on element counting:
.flex-50 {
padding: 0.625rem 0;
- border-bottom: 1px solid var(--border-color, rgb(0 0 0 / 8%));
display: flex;
align-items: center;
-
- &:last-child,
- &:nth-last-child(2) {
- border-bottom: none;
- }
+
+ &:not(:last-child):not(:nth-last-child(2)) {
+ border-bottom: 1px solid var(--border-color, rgb(0 0 0 / 8%));
+ }
}Alternatively, consider applying borders only between label-value pairs or using a different visual separator that doesn't depend on element position.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
src/app/navigation/office-navigation/office-navigation.component.scss around
lines 72-75: the current selector (&:last-child, &:nth-last-child(2)) assumes
fixed element count and fails when officeData.externalId or
officeData.parentName are conditionally absent; refactor the CSS to stop relying
on positional nth-last-child logic — instead apply borders to every item and
remove the border only for the true last logical row (e.g., wrap each
label/value pair in a consistent row container and use a selector like
"row:not(:last-child)" to keep separators between rows), or apply borders
between items (using border-top on items except the first) so conditional
rendering doesn't change which elements get borders; ensure the DOM structure
for rows is consistent so the selector targets the logical rows rather than raw
child positions.
shubhamkumar9199
force-pushed
the
fix/office-navigation-page
branch
from
f71ffbe to
eb11a63
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/app/navigation/office-navigation/office-navigation.component.scss (2)
1-8: Remove unused CSS variable.The
--header-subtitle-colorvariable (line 4) is defined but never used in the stylesheet, sinceMatCardSubtitlewas removed from the component. This is dead code.Apply this diff to remove the unused variable:
:host { --header-icon-color: #000; --header-title-color: #000; - --header-subtitle-color: #000; --field-label-color: #212121; --field-value-color: #000; --border-color: #ddd; }Don't forget to also remove it from the dark theme override at line 104.
41-63: Border logic may not work correctly in single-column responsive mode.The selector
:not(:last-child, :nth-last-child(2))correctly removes borders from the last row in 2-column grid mode. However, in single-column mode (width ≤ 768px), this removes borders from the last two individual elements regardless of whether they form a logical label-value pair, potentially creating visual inconsistencies where a border appears between a label and its value.A past review comment flagged this same concern. Consider refactoring to use a row-based approach (wrapping each label-value pair in a container) or applying borders between logical rows rather than individual elements.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/app/navigation/navigation.component.scss(1 hunks)src/app/navigation/office-navigation/office-navigation.component.html(2 hunks)src/app/navigation/office-navigation/office-navigation.component.scss(1 hunks)src/app/navigation/office-navigation/office-navigation.component.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/app/navigation/navigation.component.scss
🧰 Additional context used
📓 Path-based instructions (1)
src/app/**
⚙️ CodeRabbit configuration file
src/app/**: For Angular code: verify component separation, trackBy on *ngFor,
strict type safety, and clean observable patterns.
Files:
src/app/navigation/office-navigation/office-navigation.component.htmlsrc/app/navigation/office-navigation/office-navigation.component.tssrc/app/navigation/office-navigation/office-navigation.component.scss
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run Lint, Build and Deploy
🔇 Additional comments (7)
src/app/navigation/office-navigation/office-navigation.component.ts (1)
3-3: LGTM! Clean import consolidation.The removal of
MatCardSubtitleis correct since it's no longer used in the template, and consolidating the Material Card imports into a single line improves code organization.src/app/navigation/office-navigation/office-navigation.component.html (2)
1-12: LGTM! Header structure simplified effectively.The header layout has been streamlined by removing the external ID from the subtitle area, which aligns with the component's removal of
MatCardSubtitle.
16-22: LGTM! External ID correctly repositioned.Moving the external ID into the content area as a conditional label-value pair maintains consistency with the other fields and properly handles cases where
officeData.externalIdis undefined.src/app/navigation/office-navigation/office-navigation.component.scss (4)
10-39: LGTM! Well-structured header styling.The header layout with flexbox, icon styling, and responsive typography creates a clean and consistent visual hierarchy. The CSS custom properties enable effective theming support.
65-81: LGTM! Clear label and value styling.The distinct styling for labels (
.mat-body-strong) and values creates good visual hierarchy and readability. The typography choices are appropriate.
83-99: LGTM! Appropriate mobile optimizations.The responsive adjustments for smaller screens (≤480px) improve usability on mobile devices by reducing padding and font sizes appropriately.
101-108: LGTM! Comprehensive dark theme support.The dark theme overrides provide proper contrast and readability for dark mode users.
steinwinde
left a comment
steinwinde
left a comment
There was a problem hiding this comment.
Sorry, but I don't like this fix for many reasons:
Labels are rendered in capitals only. Why? I don't see this in any similar forms in our app.
Underlines are rendered both below the labels and (separately) below their values. I neither see this in similar forms.
We really should use the same CSS again and again, if possible - to the maximum extent possible.
Our forms are and should remain simple - in their CSS too. Why do we need CSS variables here? This looks like unnecessary overhead. Just declare two classes.
Whatever you do, please look how this is done elsewhere. E.g. if you introduce "transform: scale(x)", please make sure that this behaves exactly the same way it behaves elsewhere, e.g. not 1.05, but 1.1. (But I'd prefer we rather not do this at all.) E.g. if you introduce a colour "#212121", ask yourself, if we used the colour elsewhere - and if not, why should we use it here?! The user shouldn't see different colour schemes in each form, but one single colour scheme across all forms. I haven't checked your other CSS values, but they should not be specific for this given form.
The Jira-Ticket pointed out a layout bug. Please don't introduce fancy things that we can't do consistently across the application, because we already have a todo list that is too long.
|
@steinwinde Sorry about that — this issue won’t happen again. If it appears in any of my previous PRs, I’ll review and correct it to make sure it doesn’t repeat |
improve the broder logic improve the error
shubhamkumar9199
force-pushed
the
fix/office-navigation-page
branch
from
eb11a63 to
bb9d07f
Compare
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)
src/app/navigation/office-navigation/office-navigation.component.ts (1)
13-21: Add MatCardContent to the imports array.
MatCardContentis imported from@angular/material/cardon line 3 but is missing from the component'simportsarray. Since this is a standalone component and the template uses<mat-card-content>, you must includeMatCardContentin the imports array to avoid a runtime error.Apply this diff to fix the issue:
imports: [ ...STANDALONE_SHARED_IMPORTS, MatCardHeader, FaIconComponent, MatCardTitleGroup, MatCardTitle, + MatCardContent, ExternalIdentifierComponent, DateFormatPipe ]
🧹 Nitpick comments (1)
src/app/navigation/office-navigation/office-navigation.component.html (1)
16-22: Consider consolidating duplicate conditional checks.Lines 16-18 and 20-22 both use
*ngIf="officeData.externalId"to conditionally render the external ID label and value. While this works correctly, wrapping both elements in a single conditional container would be more efficient and reduce duplication.Consider this refactor using
ng-container:- <div class="flex-50 mat-body-strong" *ngIf="officeData.externalId"> + <ng-container *ngIf="officeData.externalId"> + <div class="flex-50 mat-body-strong"> - {{ 'labels.inputs.External Id' | translate }} - </div> - - <div class="flex-50" *ngIf="officeData.externalId"> + {{ 'labels.inputs.External Id' | translate }} + </div> + <div class="flex-50"> - <mifosx-external-identifier externalId="{{ officeData.externalId }}"></mifosx-external-identifier> - </div> + <mifosx-external-identifier externalId="{{ officeData.externalId }}"></mifosx-external-identifier> + </div> + </ng-container>
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/app/navigation/navigation.component.scss(1 hunks)src/app/navigation/office-navigation/office-navigation.component.html(2 hunks)src/app/navigation/office-navigation/office-navigation.component.scss(1 hunks)src/app/navigation/office-navigation/office-navigation.component.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/app/navigation/office-navigation/office-navigation.component.scss
🧰 Additional context used
📓 Path-based instructions (1)
src/app/**
⚙️ CodeRabbit configuration file
src/app/**: For Angular code: verify component separation, trackBy on *ngFor,
strict type safety, and clean observable patterns.
Files:
src/app/navigation/office-navigation/office-navigation.component.tssrc/app/navigation/office-navigation/office-navigation.component.htmlsrc/app/navigation/navigation.component.scss
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run Lint, Build and Deploy
🔇 Additional comments (3)
src/app/navigation/office-navigation/office-navigation.component.html (1)
1-12: Header refactoring improves layout consistency.The header structure has been simplified by removing the subtitle and using
layout-row headerclasses. This change aligns well with moving the external ID to the content area and improves the visual hierarchy.src/app/navigation/navigation.component.scss (2)
15-18: I need to examine the SCSS definitions to determine if these classes are intentionally different or if there's an actual inconsistency.These flex classes are intentionally separate and correctly used.
The
.flex-48class innavigation.component.scssis specifically for the main navigation component (navigation.component.html), while.flex-50is a separate utility defined inoffice-navigation.component.scssand used across multiple child navigation components. The 48% versus 50% difference is a common pattern when layout requires spacing between items—48% (or something similar that is equal to or less than 50%) is used when accounting for gaps or margins between flex items. This is not a naming inconsistency but rather a deliberate design choice for different layout contexts.
39-43: Verify your project's browser support requirements against modern Media Queries Level 4 syntax.The modern range syntax is supported in Chrome 104+, Firefox, and Safari 16.4+. If your project only targets modern browsers, the current code is compatible. However, if you need to support older browsers or have specific legacy browser requirements, use the traditional syntax (
@media (min-width: 768px)) instead.Without explicit browser support requirements stated in the codebase, please confirm whether the modern syntax aligns with your project's constraints.
2 participants
Description
Improved the CSS styling of the Office Navigation page to ensure proper alignment of icon.
This change enhances the visual consistency and layout across the navigation section, improving user experience.
#{Issue Number}
WEB-378
Screenshots, if any
before:
after:
Checklist
Please make sure these boxes are checked before submitting your pull request - thanks!
If you have multiple commits please combine them into one commit by squashing them.
[ X ] Read and understood the contribution guidelines at
web-app/.github/CONTRIBUTING.md.Summary by CodeRabbit
Style
Refactor