WEB-633: Fix Model Dialog for printing button #3059
Conversation
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
PDF Modal UI Enhancement src/app/clients/clients-view/general-tab/general-tab.component.html |
Added accessibility attributes (role="dialog", aria-modal="true", aria-label); implemented clickable overlay to close modal with click propagation prevention on modal content; introduced PDF download link in header; replaced embed with iframe for PDF rendering; maintained close button functionality |
PDF Modal Styling src/app/clients/clients-view/general-tab/general-tab.component.scss |
Added complete modal styling for .pdf-modal-overlay, .pdf-modal-content, .pdf-modal-header, .pdf-download, .pdf-modal-close, and .pdf-iframe with full-screen overlay, centered modal positioning, z-indexing, and visual styling |
Estimated code review effort
🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
- WEB-627 Implement Report Service in the Loan Application Print Icon #3049: Introduced the initial PDF modal with component properties (
showPdf,pdfUrl,rawPdfUrl) and methods (closePdf) that this PR's template and styling changes build upon.
Suggested reviewers
- IOhacker
🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Title check | The title references a 'printing button' fix, but the actual changes implement a PDF viewer modal with accessibility features and download functionality, not specifically fixing a printing button. | Update the title to accurately reflect the main changes, such as 'WEB-633: Add PDF viewer modal with download and accessibility features' or similar. |
✅ Passed checks (2 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| 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.
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: 1
🤖 Fix all issues with AI agents
In `@src/app/clients/clients-view/general-tab/general-tab.component.html`:
- Around line 898-926: Add keyboard accessibility by wiring the Escape key to
the modal close handler: attach (keydown.escape)="closePdf()" to the overlay
container (the element that is conditionally shown via showPdf) and ensure the
overlay is focusable when opened (e.g., manage focus in the component that sets
showPdf and calls closePdf()). Replace the hardcoded download="report.pdf" on
the anchor that uses rawPdfUrl with a dynamic property (e.g.,
download="{{downloadFileName}}" or [attr.download]="downloadFileName") provided
by the component and construct downloadFileName in the component (use
loanId/date or other identifier where pdfUrl/rawPdfUrl are created, note these
values are produced using bypassSecurityTrustResourceUrl in the code that
creates pdfUrl/rawPdfUrl). Ensure closePdf() still stops propagation and handles
keyboard closure consistently.
| <div *ngIf="showPdf" class="pdf-modal-overlay" (click)="closePdf()"> | ||
| <div | ||
| class="pdf-modal-content" | ||
| role="dialog" | ||
| aria-modal="true" | ||
| aria-label="PDF preview" | ||
| (click)="$event.stopPropagation()" | ||
| > | ||
| <button mat-icon-button class="pdf-modal-close" (click)="closePdf()" aria-label="Close PDF"> | ||
| <span aria-hidden="true">×</span> | ||
| </button> | ||
| <embed [src]="pdfUrl" type="application/pdf" width="100%" height="600px" /> | ||
|
|
||
| <div class="pdf-modal-header"> | ||
| <a | ||
| class="pdf-download" | ||
| *ngIf="rawPdfUrl" | ||
| [href]="rawPdfUrl" | ||
| download="report.pdf" | ||
| target="_blank" | ||
| rel="noopener" | ||
| > | ||
| {{ 'labels.buttons.Download' | translate }} | ||
| </a> | ||
| </div> | ||
|
|
||
| <!-- Use iframe (better cross-browser support) --> | ||
| <iframe *ngIf="pdfUrl" [src]="pdfUrl" class="pdf-iframe" frameborder="0" title="PDF preview"></iframe> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for the component TypeScript file and pdfUrl sanitization
find src/app/clients/clients-view/general-tab -name "*.component.ts" | head -1Repository: openMF/web-app
Length of output: 123
🏁 Script executed:
# Search for pdfUrl sanitization and DomSanitizer usage
rg -n -C5 'pdfUrl|DomSanitizer|bypassSecurityTrust' src/app/clients/clients-view/general-tab/Repository: openMF/web-app
Length of output: 8062
🏁 Script executed:
# Search for keyboard event handlers (Escape key, focus trap) in the component
rg -n 'keydown\.escape|keydown\.Escape|FocusMonitor|cdkTrapFocus|@HostListener.*keydown' src/app/clients/clients-view/general-tab/Repository: openMF/web-app
Length of output: 40
🏁 Script executed:
# Check for closePdf method and any keyboard handling
rg -n -A10 'closePdf|showPdf' src/app/clients/clients-view/general-tab/ --type typescriptRepository: openMF/web-app
Length of output: 92
🏁 Script executed:
# Search for all methods in the component to check for keyboard handlers
rg -n "closePdf\|@HostListener\|keydown\|escape" src/app/clients/clients-view/general-tab/general-tab.component.ts -A5Repository: openMF/web-app
Length of output: 40
🏁 Script executed:
# Search for download filename or report naming patterns
rg -n "download|report\.pdf|filename" src/app/clients/clients-view/general-tab/general-tab.component.tsRepository: openMF/web-app
Length of output: 40
🏁 Script executed:
# Check the full context of the openLoanApplicationReport method
sed -n '83,120p' src/app/clients/clients-view/general-tab/general-tab.component.tsRepository: openMF/web-app
Length of output: 1569
Consider keyboard accessibility improvements and making the download filename dynamic.
The modal implementation has good accessibility attributes and proper URL sanitization is already in place (bypassSecurityTrustResourceUrl on line 117). Two areas for enhancement:
-
Keyboard accessibility: Add an Escape key handler to close the modal. Add
(keydown.escape)="closePdf()"to the modal overlay div for keyboard users. -
Download filename: The hardcoded
download="report.pdf"could be made dynamic (e.g., include the loan ID or date) for better UX and easier file identification.
🤖 Prompt for AI Agents
In `@src/app/clients/clients-view/general-tab/general-tab.component.html` around
lines 898 - 926, Add keyboard accessibility by wiring the Escape key to the
modal close handler: attach (keydown.escape)="closePdf()" to the overlay
container (the element that is conditionally shown via showPdf) and ensure the
overlay is focusable when opened (e.g., manage focus in the component that sets
showPdf and calls closePdf()). Replace the hardcoded download="report.pdf" on
the anchor that uses rawPdfUrl with a dynamic property (e.g.,
download="{{downloadFileName}}" or [attr.download]="downloadFileName") provided
by the component and construct downloadFileName in the component (use
loanId/date or other identifier where pdfUrl/rawPdfUrl are created, note these
values are produced using bypassSecurityTrustResourceUrl in the code that
creates pdfUrl/rawPdfUrl). Ensure closePdf() still stops propagation and handles
keyboard closure consistently.
1 participant
Description
WEB-633: Fix Model Dialog for printing button
Related issues and discussion
#{Issue Number}
Screenshots, if any
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
✏️ Tip: You can customize this high-level summary in your review settings.