WEB-(356)-improve the css of general-tab #2716
Conversation
WalkthroughThe pull request refactors the office view layout to a more compact, detail-focused design. Changes include HTML restructuring with repositioned buttons, added section dividers, modified external ID rendering, and extensive CSS updates for spacing, layout utilities, responsive behavior, and card styling. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes The changes involve multiple files with straightforward styling and layout updates, along with minor HTML restructuring. While the scope is reasonable and most modifications follow consistent patterns (spacing adjustments, new utility classes), reviewers should verify responsive behavior across viewport sizes, button accessibility after repositioning, and component rendering logic for the external ID conditional display. Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/organization/offices/view-office/view-office.component.html (1)
15-27: *Add trackBy function to ngFor loop for better performance.The *ngFor loop iterating over
officeDatatables(line 15) is missing a trackBy function. Without it, Angular re-renders the entire list on each change cycle, causing unnecessary DOM operations.Add a trackBy function to the component:
trackByTableName(index: number, item: any): string { return item.registeredTableName; }Then update the template to reference it:
- <span *ngFor="let officeDatatable of officeDatatables"> + <span *ngFor="let officeDatatable of officeDatatables; trackBy: trackByTableName">
🧹 Nitpick comments (3)
src/app/organization/offices/view-office/general-tab/general-tab.component.html (1)
22-27: *Simplify external ID conditional rendering using ngIf-else.The dual *ngIf pattern for rendering external ID or "Unassigned" can be simplified with Angular's *ngIf-else directive to reduce redundant markup.
<div class="flex-45 mat-body-strong left">{{ 'labels.inputs.External Id' | translate }}</div> - <div class="flex-50 right" *ngIf="officeData.externalId"> + <div class="flex-50 right" *ngIf="officeData.externalId; else noExternalId"> <mifosx-external-identifier externalId="{{ officeData.externalId }}"></mifosx-external-identifier> </div> - <div class="flex-50 right" *ngIf="!officeData.externalId"> + <ng-template #noExternalId> + <div class="flex-50 right"> - {{ 'labels.inputs.Unassigned' | translate }} - </div> + {{ 'labels.inputs.Unassigned' | translate }} + </div> + </ng-template>src/app/organization/offices/view-office/view-office.component.scss (1)
43-46: Consider adding additional responsive breakpoints for better mobile coverage.The current media query only targets devices with width ≤ 768px. Consider adding breakpoints for smaller screens (e.g., 480px, 600px) to ensure the layout remains usable on various mobile devices and tablets.
For example:
// Mobile-first: additional breakpoints @media (width <= 480px) { .office-card { width: 95%; padding: 0.3rem; } } @media (width <= 600px) { // Tablet-specific adjustments }src/app/organization/offices/view-office/general-tab/general-tab.component.scss (1)
80-97: Clarify the distinction between container width constraints.The stylesheet defines multiple width constraint classes with overlapping purposes:
.compact-view(max-width: 450px).narrow-container(max-width: 600px, in view-office.component.scss).tab-container.compact-view(max-width: 450px, lines 109-112)The purpose and intended usage of each is unclear. Consider adding comments to document when to use which class, or consolidate if they serve identical purposes.
Example documentation:
// Use for the general tab's compact detail view .compact-view { max-width: 450px; // ... } // Use for the outer office view container (wider than general tab) .narrow-container { max-width: 600px; // ... }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/app/organization/offices/view-office/general-tab/general-tab.component.html(2 hunks)src/app/organization/offices/view-office/general-tab/general-tab.component.scss(2 hunks)src/app/organization/offices/view-office/view-office.component.html(2 hunks)src/app/organization/offices/view-office/view-office.component.scss(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/organization/offices/view-office/general-tab/general-tab.component.htmlsrc/app/organization/offices/view-office/view-office.component.htmlsrc/app/organization/offices/view-office/view-office.component.scsssrc/app/organization/offices/view-office/general-tab/general-tab.component.scss
🔇 Additional comments (1)
src/app/organization/offices/view-office/general-tab/general-tab.component.html (1)
30-36: Button relocation and styling updates look good.The repositioning of the Edit button to the bottom-button-container with updated icon spacing and permission gating is clean and aligns with the compact UI design.
| .m-b-10 { | ||
| margin-bottom: 10px; | ||
| } | ||
|
|
||
| .m-r-5 { | ||
| margin-right: 5px; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Consider centralizing margin utility classes.
The generic margin utilities .m-b-10 and .m-r-5 are defined within this component-scoped SCSS file. If these are used across multiple components or if a global utility system already exists, consider:
- Moving these to a shared utilities file (e.g.,
src/styles/utilities.scssor similar) - Checking if a Tailwind CSS or Bootstrap utility system already provides these
This will improve maintainability and prevent duplicate utility definitions across the codebase.
Run the following script to check if these utilities exist elsewhere:
🏁 Script executed:
#!/bin/bash
# Search for duplicate margin utility definitions
echo "=== Searching for .m-r-5 definition ==="
rg -nA2 '\.m-r-5\s*\{' --type scss
echo "=== Searching for .m-b-10 definition ==="
rg -nA2 '\.m-b-10\s*\{' --type scss
echo "=== Checking for global utilities file ==="
fd -e scss -e css | grep -iE 'utilities|common|global|variables' | head -10Length of output: 508
Now I'll run a corrected script to search for the margin utilities in the codebase.
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for margin utility definitions using corrected approach
echo "=== Searching for .m-r-5 definitions (glob pattern) ==="
rg -nA2 '\.m-r-5\s*\{' -g '*.scss' -g '*.css'
echo ""
echo "=== Searching for .m-b-10 definitions (glob pattern) ==="
rg -nA2 '\.m-b-10\s*\{' -g '*.scss' -g '*.css'
echo ""
echo "=== Searching for margin utility pattern m-[rblt]-[0-9] ==="
rg -nA1 '\.m-[rblt]-\d+\s*\{' -g '*.scss' -g '*.css' | head -20
echo ""
echo "=== Checking if general-tab.component.scss has margins ==="
rg -n 'margin-(right|bottom):' src/app/organization/offices/view-office/general-tab/general-tab.component.scss
echo ""
echo "=== Checking theme variables file ==="
cat src/theme/_variables.scss 2>/dev/null | head -50 || echo "File not found or cannot be read"
Length of output: 2189
Now I'll verify how global utilities are loaded in this component and whether there are other style imports.
🏁 Script executed:
#!/bin/bash
# Verify component style configuration and global utility import
echo "=== Full general-tab.component.scss content ==="
cat src/app/organization/offices/view-office/general-tab/general-tab.component.scss
echo ""
echo "=== Checking general-tab.component.ts for styleUrls ==="
rg -n 'styleUrls|styles:' src/app/organization/offices/view-office/general-tab/general-tab.component.ts -A3 -B1
echo ""
echo "=== Checking if global styles are imported in angular.json or main.ts ==="
grep -r '_margin\.scss' src/ 2>/dev/null | head -5
echo ""
echo "=== Checking styles.scss for global utility imports ==="
cat src/styles.scss 2>/dev/null | grep -E '@import.*margin|@include' | head -10Length of output: 1902
Remove duplicate margin utilities from component SCSS.
The utilities .m-b-10 and .m-r-5 (lines 39-45) are duplicated. They already exist in src/assets/styles/_margin.scss with identical definitions. Remove these lines and either:
- Import the global utilities file:
@import 'src/assets/styles/margin'; - Or ensure global styles are accessible to this component
This eliminates code duplication and aligns with the established utility system.
🤖 Prompt for AI Agents
In
src/app/organization/offices/view-office/general-tab/general-tab.component.scss
around lines 39 to 45, the `.m-b-10` and `.m-r-5` margin utility rules are
duplicates of the global utilities in src/assets/styles/_margin.scss; remove
these two rule blocks from this component stylesheet and either add an import of
the shared utilities (e.g., import the global margin utilities file from
src/assets/styles/margin) at the top of this component SCSS or ensure the global
styles are included via the project/global style pipeline so the component uses
the single source of truth for margin utilities.
1 participant
Description
Enhanced the CSS for the button to improve spacing, padding, and alignment.
#{Issue Number}
WEB-(356)
Screenshots, if any
before:
after:
Checklist
Please make sure these boxes are checked before submitting your pull request - thanks!
[ X ] 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