WEB-603: Improve styling & UX of bulk import#3306
Merged
IOhacker merged 1 commit intoopenMF:devfrom Mar 6, 2026
Merged
Conversation
|
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:
Note
|
| Cohort / File(s) | Summary |
|---|---|
Bulk Import Component (template, logic, styles) src/app/organization/bulk-import/bulk-import.component.html, src/app/organization/bulk-import/bulk-import.component.ts, src/app/organization/bulk-import/bulk-import.component.scss |
Introduces public bulkImportOptions (17 option objects). Replaces many static <mat-list-item> blocks with two *ngFor loops (slice(0,9) and slice(9)) rendering option.permission, option.icon, option.routerLink, option.title, option.explanation. Adds data-id attributes, updates click/toggle handling to use arrowBooleans[option.index] and arrowBooleansToggle(option.index). Adds comprehensive SCSS for two-column layout, spacing, hover/rotation, responsive widths, and dark-mode overrides. |
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested reviewers
- alberto-art3ch
- IOhacker
🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title accurately reflects the main changes: refactoring bulk import UI with improved styling (SCSS additions), UX enhancements (two-column layout, toggleable explanations), and better data organization. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
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 help to get the list of available commands and usage tips.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/organization/bulk-import/bulk-import.component.html`:
- Around line 18-33: Replace the non-semantic clickable divs with proper
interactive elements: change the left navigation wrapper (class
"menu-left-section" using [routerLink]="[option.routerLink]") into an anchor
element so routing is keyboard-accessible, and change the right toggle wrapper
(class "menu-right-section") into a <button type="button"> that calls
arrowBooleansToggle(option.index) (keep the $event.stopPropagation() behavior)
and add aria-expanded="{{ arrowBooleans[option.index] }}"; ensure the bindings
for option.icon, option.title, option.explanation, option.routerLink and the
arrowBooleans/arrowBooleansToggle logic remain intact and that the visual
classes (e.g., [class.rotated]) are preserved.
In `@src/app/organization/bulk-import/bulk-import.component.scss`:
- Line 76: The rule currently uses the deprecated declaration "word-break:
break-word;"; update that CSS rule by replacing the deprecated value with a
supported combination: add "overflow-wrap: anywhere;" (or "overflow-wrap:
break-word;" if you prefer softer wrapping) and set "word-break" to a supported
value such as "normal" (e.g., replace "word-break: break-word;" with
"overflow-wrap: anywhere; word-break: normal;") so the styling is modern and
cross-browser compatible.
In `@src/app/organization/bulk-import/bulk-import.component.ts`:
- Around line 47-53: The explanation text for the menu items is incorrect: the
object with permission 'READ_USER' / routerLink 'Users' / title
'labels.heading.Users' incorrectly references offices/loan accounts, and the
similar block at lines 103-109 (permission 'READ_EMPLOYEE' / title
'labels.heading.Employees') also has the wrong entity; update the
explanation/localization keys so the Users item explains downloading/uploading
user templates (e.g., change the explanation key from offices to users or to a
proper labels.text.UsersUpload description) and similarly update the Employees
item to reference employee templates/uploads, ensuring the explanation keys
match the intent of 'labels.heading.Users' and 'labels.heading.Employees'.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0fb0969f-8318-49a0-a434-95c16c6aae87
📒 Files selected for processing (3)
src/app/organization/bulk-import/bulk-import.component.htmlsrc/app/organization/bulk-import/bulk-import.component.scsssrc/app/organization/bulk-import/bulk-import.component.ts
Comment on lines
+18
to
+33
| <div class="menu-left-section" [routerLink]="[option.routerLink]"> | ||
| <mat-icon matListIcon> | ||
| <fa-icon [icon]="option.icon" size="sm"></fa-icon> | ||
| </mat-icon> | ||
| <div class="menu-text-content"> | ||
| <div class="menu-title">{{ option.title | translate }}</div> | ||
| @if (arrowBooleans[option.index]) { | ||
| <p class="menu-explanation" [routerLink]="[option.routerLink]"> | ||
| {{ option.explanation | translate }} | ||
| </p> | ||
| } | ||
| </div> | ||
| </div> | ||
| <div class="menu-right-section" (click)="$event.stopPropagation(); arrowBooleansToggle(option.index)"> | ||
| <fa-icon [icon]="'arrow-down'" [class.rotated]="arrowBooleans[option.index]" size="md"></fa-icon> | ||
| </div> |
There was a problem hiding this comment.
Use semantic interactive elements for navigation/toggle controls.
Lines 18/48 and 31/61 use clickable <div> blocks for navigation/toggling. This is not keyboard-friendly and creates an accessibility blocker. Use <a> for routing and <button type="button"> for toggle actions with aria-expanded.
♿ Suggested semantic markup update
- <div class="menu-left-section" [routerLink]="[option.routerLink]">
+ <a class="menu-left-section" [routerLink]="[option.routerLink]">
...
- </div>
+ </a>
- <div class="menu-right-section" (click)="$event.stopPropagation(); arrowBooleansToggle(option.index)">
+ <button
+ type="button"
+ class="menu-right-section"
+ [attr.aria-expanded]="arrowBooleans[option.index]"
+ (click)="$event.stopPropagation(); arrowBooleansToggle(option.index)">
<fa-icon [icon]="'arrow-down'" [class.rotated]="arrowBooleans[option.index]" size="md"></fa-icon>
- </div>
+ </button>Also applies to: 48-63
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/organization/bulk-import/bulk-import.component.html` around lines 18
- 33, Replace the non-semantic clickable divs with proper interactive elements:
change the left navigation wrapper (class "menu-left-section" using
[routerLink]="[option.routerLink]") into an anchor element so routing is
keyboard-accessible, and change the right toggle wrapper (class
"menu-right-section") into a <button type="button"> that calls
arrowBooleansToggle(option.index) (keep the $event.stopPropagation() behavior)
and add aria-expanded="{{ arrowBooleans[option.index] }}"; ensure the bindings
for option.icon, option.title, option.explanation, option.routerLink and the
arrowBooleans/arrowBooleansToggle logic remain intact and that the visual
classes (e.g., [class.rotated]) are preserved.
Comment on lines
+47
to
+53
| { | ||
| permission: 'READ_USER', | ||
| routerLink: 'Users', | ||
| icon: 'user', | ||
| title: 'labels.heading.Users', | ||
| explanation: 'labels.text.Download offices template and Upload office excel files', | ||
| index: 1 |
There was a problem hiding this comment.
Fix mismatched explanation text for Users and Employees.
Line 52 and Line 108 describe the wrong entities (offices/loan accounts), which will mislead users during import flows.
✏️ Suggested content correction
- explanation: 'labels.text.Download offices template and Upload office excel files',
+ explanation: 'labels.text.Download users template and upload users excel files',
...
- explanation: 'labels.text.Download loan accounts template and upload loan account excel files',
+ explanation: 'labels.text.Download employees template and upload employees excel files',Also applies to: 103-109
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/organization/bulk-import/bulk-import.component.ts` around lines 47 -
53, The explanation text for the menu items is incorrect: the object with
permission 'READ_USER' / routerLink 'Users' / title 'labels.heading.Users'
incorrectly references offices/loan accounts, and the similar block at lines
103-109 (permission 'READ_EMPLOYEE' / title 'labels.heading.Employees') also has
the wrong entity; update the explanation/localization keys so the Users item
explains downloading/uploading user templates (e.g., change the explanation key
from offices to users or to a proper labels.text.UsersUpload description) and
similarly update the Employees item to reference employee templates/uploads,
ensuring the explanation keys match the intent of 'labels.heading.Users' and
'labels.heading.Employees'.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
iramsk02
force-pushed
the
WEB-603/improve-styling-of-bulk-import
branch
from
0e86dd1 to
54973cb
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/app/organization/bulk-import/bulk-import.component.html (1)
31-36:⚠️ Potential issue | 🟠 MajorToggle button still needs explicit semantics for accessibility.
This is better than the previous clickable
<div>, but the icon-only<button>still lackstype="button",aria-expanded, and an accessible label/relationship to the expanded text.♿ Suggested fix
- <button - class="menu-right-section" - (click)="$event.stopPropagation(); arrowBooleansToggle(option.index)" - > + <button + type="button" + class="menu-right-section" + [attr.aria-expanded]="arrowBooleans[option.index]" + [attr.aria-controls]="'bulk-import-explanation-' + option.index" + [attr.aria-label]="(arrowBooleans[option.index] ? 'Collapse ' : 'Expand ') + (option.title | translate)" + (click)="$event.stopPropagation(); arrowBooleansToggle(option.index)" + > <fa-icon [icon]="'arrow-down'" [class.rotated]="arrowBooleans[option.index]" size="md"></fa-icon> </button>- <p class="menu-explanation" [routerLink]="[option.routerLink]"> + <p class="menu-explanation" [id]="'bulk-import-explanation-' + option.index" [routerLink]="[option.routerLink]"> {{ option.explanation | translate }} </p>Also applies to: 64-69
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/organization/bulk-import/bulk-import.component.html` around lines 31 - 36, The icon-only toggle button lacks explicit semantics: update the button used with arrowBooleansToggle(option.index) to include type="button", bind aria-expanded to arrowBooleans[option.index], and add an accessible label or relationship (either aria-label describing the action or aria-controls linking to the ID of the expandable panel/text for that option); do the same for the other instance using the same pattern so screen readers can detect state and target (references: arrowBooleansToggle, arrowBooleans, option.index).
🧹 Nitpick comments (2)
src/app/organization/bulk-import/bulk-import.component.html (2)
14-40: Left/right column markup is duplicated; extract shared item template.Both columns repeat the same item structure, which increases drift risk (especially for accessibility/state changes). Extracting a shared
ng-template(or a small presentational child component) would keep behavior consistent.Also applies to: 47-73
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/organization/bulk-import/bulk-import.component.html` around lines 14 - 40, The list item markup for each bulk import option is duplicated; extract it into a shared ng-template or small presentational child component to avoid drift. Create a reusable template/component that accepts inputs: option (from bulkImportOptions), arrowState (arrowBooleans[option.index]) and an output for toggling (arrowBooleansToggle(option.index)), move the repeated structure (routerLink, mat-list-item, fa-icon, menu-title, conditionally-rendered menu-explanation using arrowState, and the button click handler) into that template/component, then replace both duplicated blocks with a single invocation (ngTemplateOutlet or the child component) wired to bulkImportOptions, option.index, mifosxHasPermission, option.routerLink and option.permission so behavior and accessibility remain identical.
14-16: Template binding depends on weakly typed option data; tighten the option contract.This template assumes
option.permission,option.routerLink, andoption.indexare always valid. With the current cross-file setup (bulkImportOptionsuntyped literal + directive input typed asany), permission/key typos stay runtime-only.Consider introducing a
BulkImportOptioninterface (or readonly typed model) and changingmifosxHasPermissioninput fromanytostringinsrc/app/directives/has-permission/has-permission.directive.ts.As per coding guidelines, "
src/app/**: For Angular code: verify component separation, trackBy on *ngFor, strict type safety, and clean observable patterns."Also applies to: 47-50
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/organization/bulk-import/bulk-import.component.html` around lines 14 - 16, The template uses untyped option objects (bulkImportOptions) and a loosely typed directive input, causing runtime failures when keys are mistyped; define a BulkImportOption interface (readonly fields: permission: string, routerLink: string, index: number, plus any other used props) and type the bulkImportOptions array with it, update the component to use that typed array, and change the mifosxHasPermission directive's input in has-permission.directive.ts from any to string so the template binding *mifosxHasPermission="option.permission" is type-checked; also ensure the *ngFor track uses the strongly-typed index field (track option.index) or replace with trackBy function if needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/app/organization/bulk-import/bulk-import.component.html`:
- Around line 31-36: The icon-only toggle button lacks explicit semantics:
update the button used with arrowBooleansToggle(option.index) to include
type="button", bind aria-expanded to arrowBooleans[option.index], and add an
accessible label or relationship (either aria-label describing the action or
aria-controls linking to the ID of the expandable panel/text for that option);
do the same for the other instance using the same pattern so screen readers can
detect state and target (references: arrowBooleansToggle, arrowBooleans,
option.index).
---
Nitpick comments:
In `@src/app/organization/bulk-import/bulk-import.component.html`:
- Around line 14-40: The list item markup for each bulk import option is
duplicated; extract it into a shared ng-template or small presentational child
component to avoid drift. Create a reusable template/component that accepts
inputs: option (from bulkImportOptions), arrowState
(arrowBooleans[option.index]) and an output for toggling
(arrowBooleansToggle(option.index)), move the repeated structure (routerLink,
mat-list-item, fa-icon, menu-title, conditionally-rendered menu-explanation
using arrowState, and the button click handler) into that template/component,
then replace both duplicated blocks with a single invocation (ngTemplateOutlet
or the child component) wired to bulkImportOptions, option.index,
mifosxHasPermission, option.routerLink and option.permission so behavior and
accessibility remain identical.
- Around line 14-16: The template uses untyped option objects
(bulkImportOptions) and a loosely typed directive input, causing runtime
failures when keys are mistyped; define a BulkImportOption interface (readonly
fields: permission: string, routerLink: string, index: number, plus any other
used props) and type the bulkImportOptions array with it, update the component
to use that typed array, and change the mifosxHasPermission directive's input in
has-permission.directive.ts from any to string so the template binding
*mifosxHasPermission="option.permission" is type-checked; also ensure the *ngFor
track uses the strongly-typed index field (track option.index) or replace with
trackBy function if needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2a2380b8-d379-447b-b4ce-47798b5b5363
📒 Files selected for processing (3)
src/app/organization/bulk-import/bulk-import.component.htmlsrc/app/organization/bulk-import/bulk-import.component.scsssrc/app/organization/bulk-import/bulk-import.component.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/app/organization/bulk-import/bulk-import.component.ts
- src/app/organization/bulk-import/bulk-import.component.scss
iramsk02
force-pushed
the
WEB-603/improve-styling-of-bulk-import
branch
from
54973cb to
7dbd373
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/app/organization/bulk-import/bulk-import.component.ts (1)
64-69:⚠️ Potential issue | 🟡 MinorFix swapped explanation text for Loan Accounts and Employees.
Line 68 describes Employees under Loan Accounts, and Line 108 describes Loan Accounts under Employees. These labels are reversed and will confuse import actions.
✏️ Suggested fix
{ permission: 'READ_LOAN', routerLink: 'Loan Accounts', icon: 'money-bill-alt', title: 'labels.heading.Loan Accounts', - explanation: 'labels.text.Download employees template and upload employees excel files', + explanation: 'labels.text.Download loan accounts template and upload loan account excel files', index: 3 }, @@ { permission: 'READ_STAFF', routerLink: 'Employees', icon: 'user', title: 'labels.heading.Employees', - explanation: 'labels.text.Download loan accounts template and upload loan account excel files', + explanation: 'labels.text.Download employees template and upload employees excel files', index: 8 },Also applies to: 103-109
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/organization/bulk-import/bulk-import.component.ts` around lines 64 - 69, The explanation texts for the bulk-import entries are swapped: locate the objects with routerLink: 'Loan Accounts' and routerLink: 'Employees' in bulk-import.component.ts (the entries that include title: 'labels.heading.Loan Accounts' and title: 'labels.heading.Employees') and swap their explanation property values so the Loan Accounts entry gets the Loan Accounts explanation and the Employees entry gets the Employees explanation; ensure you only swap the explanation strings and leave permission, icon, title, and index unchanged.src/app/organization/bulk-import/bulk-import.component.html (1)
18-36:⚠️ Potential issue | 🟠 MajorUse semantic navigation/toggle controls with ARIA state.
Line 18 and Line 51 use clickable
<div>elements for routing, which are not keyboard-friendly.
Line 31 and Line 64 buttons should expose state (aria-expanded) and explicit button type.♿ Suggested fix
- <div class="menu-left-section" [routerLink]="[option.routerLink]"> + <a class="menu-left-section" [routerLink]="[option.routerLink]"> <mat-icon matListIcon> <fa-icon [icon]="option.icon" size="sm"></fa-icon> </mat-icon> <div class="menu-text-content"> <div class="menu-title">{{ option.title | translate }}</div> `@if` (arrowBooleans[option.index]) { - <p class="menu-explanation" [routerLink]="[option.routerLink]"> + <p class="menu-explanation"> {{ option.explanation | translate }} </p> } </div> - </div> + </a> <button + type="button" class="menu-right-section" + [attr.aria-expanded]="arrowBooleans[option.index]" (click)="$event.stopPropagation(); arrowBooleansToggle(option.index)" > <fa-icon [icon]="'arrow-down'" [class.rotated]="arrowBooleans[option.index]" size="md"></fa-icon> </button>Also applies to: 51-69
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/organization/bulk-import/bulk-import.component.html` around lines 18 - 36, Replace non-interactive divs used for navigation with semantic interactive elements and add ARIA state: change the clickable container using [routerLink] (the element with class "menu-left-section" and binding [routerLink]="[option.routerLink]") to an <a> or <button> that supports keyboard focus and activation via Enter/Space, and ensure its accessible name comes from the "menu-title". On the toggle control, update the button (the element invoking arrowBooleansToggle(option.index)) to include type="button" and aria-expanded bound to arrowBooleans[option.index]; keep the fa-icon rotation logic but rely on aria-expanded to expose state. Also ensure any alternative toggle instance (the duplicate block using arrowBooleans and arrowBooleansToggle) receives the same changes so both navigation and toggle controls are keyboard-accessible and expose state.
🧹 Nitpick comments (1)
src/app/organization/bulk-import/bulk-import.component.html (1)
14-40: Consider extracting a reusable item template to reduce duplication.The left and right column blocks are almost identical; extracting a shared template/component will reduce future drift and simplify maintenance.
Also applies to: 47-73
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/organization/bulk-import/bulk-import.component.html` around lines 14 - 40, Extract the repeated list-item markup into a reusable Angular template or component (e.g., a new component BulkImportItem or an <ng-template> named bulkImportItemTemplate) and replace both left/right column blocks with calls to that template/component; the extracted unit should accept inputs: an option object (from bulkImportOptions), arrowBooleans and an index, and expose the click handler arrowBooleansToggle, preserve the permission directive *mifosxHasPermission, routerLink bindings, mat-icon/fa-icon usage, and the conditional paragraph that reads arrowBooleans[option.index] so behavior and bindings remain identical while eliminating duplication.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/organization/bulk-import/bulk-import.component.scss`:
- Around line 82-93: The .menu-right-section rule removed the outline making
keyboard focus invisible; restore a visible focus indicator by adding a
:focus-visible (and optionally :focus) style for .menu-right-section that
provides a clear, accessible outline or focus ring (e.g., outline or box-shadow
with outline-offset) while leaving the default mouse styling intact so keyboard
users can see the toggle control focus.
---
Duplicate comments:
In `@src/app/organization/bulk-import/bulk-import.component.html`:
- Around line 18-36: Replace non-interactive divs used for navigation with
semantic interactive elements and add ARIA state: change the clickable container
using [routerLink] (the element with class "menu-left-section" and binding
[routerLink]="[option.routerLink]") to an <a> or <button> that supports keyboard
focus and activation via Enter/Space, and ensure its accessible name comes from
the "menu-title". On the toggle control, update the button (the element invoking
arrowBooleansToggle(option.index)) to include type="button" and aria-expanded
bound to arrowBooleans[option.index]; keep the fa-icon rotation logic but rely
on aria-expanded to expose state. Also ensure any alternative toggle instance
(the duplicate block using arrowBooleans and arrowBooleansToggle) receives the
same changes so both navigation and toggle controls are keyboard-accessible and
expose state.
In `@src/app/organization/bulk-import/bulk-import.component.ts`:
- Around line 64-69: The explanation texts for the bulk-import entries are
swapped: locate the objects with routerLink: 'Loan Accounts' and routerLink:
'Employees' in bulk-import.component.ts (the entries that include title:
'labels.heading.Loan Accounts' and title: 'labels.heading.Employees') and swap
their explanation property values so the Loan Accounts entry gets the Loan
Accounts explanation and the Employees entry gets the Employees explanation;
ensure you only swap the explanation strings and leave permission, icon, title,
and index unchanged.
---
Nitpick comments:
In `@src/app/organization/bulk-import/bulk-import.component.html`:
- Around line 14-40: Extract the repeated list-item markup into a reusable
Angular template or component (e.g., a new component BulkImportItem or an
<ng-template> named bulkImportItemTemplate) and replace both left/right column
blocks with calls to that template/component; the extracted unit should accept
inputs: an option object (from bulkImportOptions), arrowBooleans and an index,
and expose the click handler arrowBooleansToggle, preserve the permission
directive *mifosxHasPermission, routerLink bindings, mat-icon/fa-icon usage, and
the conditional paragraph that reads arrowBooleans[option.index] so behavior and
bindings remain identical while eliminating duplication.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cbd38d46-e0bb-44b5-814e-4bc838283651
📒 Files selected for processing (3)
src/app/organization/bulk-import/bulk-import.component.htmlsrc/app/organization/bulk-import/bulk-import.component.scsssrc/app/organization/bulk-import/bulk-import.component.ts
iramsk02
force-pushed
the
WEB-603/improve-styling-of-bulk-import
branch
from
7dbd373 to
ebd224a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/app/organization/bulk-import/bulk-import.component.html (1)
14-40:⚠️ Potential issue | 🟠 MajorUse semantic
<a>element for navigation and addaria-expandedto toggle button.The left section still uses a
<div>with[routerLink]instead of an<a>element. This impacts keyboard navigation since<div>elements are not natively focusable. Additionally, the toggle button should includetype="button"to prevent form submission in certain contexts andaria-expandedfor screen reader users.♿ Proposed semantic markup fix
<mat-list-item *mifosxHasPermission="option.permission"> <div class="menu-list-item-content"> - <div class="menu-left-section" [routerLink]="[option.routerLink]"> + <a class="menu-left-section" [routerLink]="[option.routerLink]"> <mat-icon matListIcon> <fa-icon [icon]="option.icon" size="sm"></fa-icon> </mat-icon> <div class="menu-text-content"> <div class="menu-title">{{ option.title | translate }}</div> `@if` (arrowBooleans[option.index]) { - <p class="menu-explanation" [routerLink]="[option.routerLink]"> + <p class="menu-explanation"> {{ option.explanation | translate }} </p> } </div> - </div> + </a> <button + type="button" class="menu-right-section" + [attr.aria-expanded]="arrowBooleans[option.index]" (click)="$event.stopPropagation(); arrowBooleansToggle(option.index)" >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/organization/bulk-import/bulk-import.component.html` around lines 14 - 40, Replace the non-focusable left-section <div> that uses [routerLink] with a semantic anchor element so keyboard users can tab to the navigation (update the template where bulkImportOptions is iterated and the element with [routerLink] and data-id bound to option.routerLink), and update the toggle button (the element invoking arrowBooleansToggle(option.index)) to include type="button" to prevent accidental form submission and bind aria-expanded to the current open state (e.g., aria-expanded="{{ arrowBooleans[option.index] }}" or equivalent) so screen readers announce the expanded/collapsed state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/organization/bulk-import/bulk-import.component.html`:
- Around line 47-73: Replace the non-semantic navigation divs in the right
column loop (the block iterating bulkImportOptions.slice(9)) with proper anchor
elements for navigation (use the same [routerLink] on the anchor), remove the
redundant [routerLink] from the nested <p> that shows option.explanation, and
update the toggle button (used with arrowBooleans and arrowBooleansToggle) to
include type="button" and an aria-expanded attribute bound to
arrowBooleans[option.index] (and optionally aria-controls referencing the
explanation element id) so the markup mirrors the left column accessibility
fixes.
In `@src/app/organization/bulk-import/bulk-import.component.scss`:
- Around line 94-98: The outlined focus rule in bulk-import.component.scss uses
a quoted fallback for the CSS variable (outline: 2px solid
var(--md-sys-color-primary, '#1074b9')) which is invalid; update the
&:focus-visible rule to remove the quotes around the hex fallback so the
fallback is a valid color literal (use `#1074b9` instead of '#1074b9') ensuring
the outline displays when --md-sys-color-primary is undefined.
---
Duplicate comments:
In `@src/app/organization/bulk-import/bulk-import.component.html`:
- Around line 14-40: Replace the non-focusable left-section <div> that uses
[routerLink] with a semantic anchor element so keyboard users can tab to the
navigation (update the template where bulkImportOptions is iterated and the
element with [routerLink] and data-id bound to option.routerLink), and update
the toggle button (the element invoking arrowBooleansToggle(option.index)) to
include type="button" to prevent accidental form submission and bind
aria-expanded to the current open state (e.g., aria-expanded="{{
arrowBooleans[option.index] }}" or equivalent) so screen readers announce the
expanded/collapsed state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 863face2-4c20-438a-9aea-a24a14e3e346
📒 Files selected for processing (3)
src/app/organization/bulk-import/bulk-import.component.htmlsrc/app/organization/bulk-import/bulk-import.component.scsssrc/app/organization/bulk-import/bulk-import.component.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/app/organization/bulk-import/bulk-import.component.ts
Comment on lines
+47
to
+73
| @for (option of bulkImportOptions.slice(9); track option.index) { | ||
| <div [attr.data-id]="option.routerLink"> | ||
| <mat-list-item *mifosxHasPermission="option.permission"> | ||
| <div class="menu-list-item-content"> | ||
| <div class="menu-left-section" [routerLink]="[option.routerLink]"> | ||
| <mat-icon matListIcon> | ||
| <fa-icon [icon]="option.icon" size="sm"></fa-icon> | ||
| </mat-icon> | ||
| <div class="menu-text-content"> | ||
| <div class="menu-title">{{ option.title | translate }}</div> | ||
| @if (arrowBooleans[option.index]) { | ||
| <p class="menu-explanation" [routerLink]="[option.routerLink]"> | ||
| {{ option.explanation | translate }} | ||
| </p> | ||
| } | ||
| </div> | ||
| </div> | ||
| <button | ||
| class="menu-right-section" | ||
| (click)="$event.stopPropagation(); arrowBooleansToggle(option.index)" | ||
| > | ||
| <fa-icon [icon]="'arrow-down'" [class.rotated]="arrowBooleans[option.index]" size="md"></fa-icon> | ||
| </button> | ||
| </div> | ||
| </mat-list-item> | ||
| </div> | ||
| } |
There was a problem hiding this comment.
Apply the same semantic markup fixes to the right column.
The right column has the same accessibility issues as the left column: <div> used instead of <a> for navigation, redundant [routerLink] on the nested <p> element, and the toggle button missing type="button" and aria-expanded.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/organization/bulk-import/bulk-import.component.html` around lines 47
- 73, Replace the non-semantic navigation divs in the right column loop (the
block iterating bulkImportOptions.slice(9)) with proper anchor elements for
navigation (use the same [routerLink] on the anchor), remove the redundant
[routerLink] from the nested <p> that shows option.explanation, and update the
toggle button (used with arrowBooleans and arrowBooleansToggle) to include
type="button" and an aria-expanded attribute bound to
arrowBooleans[option.index] (and optionally aria-controls referencing the
explanation element id) so the markup mirrors the left column accessibility
fixes.
iramsk02
force-pushed
the
WEB-603/improve-styling-of-bulk-import
branch
from
ebd224a to
bb2985b
Compare
IOhacker
requested changes
Mar 6, 2026
Comment on lines
+47
to
+53
| { | ||
| permission: 'READ_USER', | ||
| routerLink: 'Users', | ||
| icon: 'user', | ||
| title: 'labels.heading.Users', | ||
| explanation: 'labels.text.Download offices template and Upload office excel files', | ||
| index: 1 |
Contributor
Author
Right now it takes this explanation from src/assests/translations/en-US.json 
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR introduces a complete redesign and refactor of the Bulk Import user interface to align with modern design standards and improve functional reliability.
Key Changes:
Related Ticket
https://mifosforge.jira.com/browse/WEB-603
Screenshots
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.
Read and understood the contribution guidelines at
web-app/.github/CONTRIBUTING.md.Summary by CodeRabbit
New Features
Style
Refactor