fix: Make it easier to hit exit in modals on mobile devices#126
fix: Make it easier to hit exit in modals on mobile devices#126
Conversation
dkorecko
changed the title
WalkthroughThe pull request updates CSS classes for close/cancel icons across three Razor components: ActionModal.razor (close icon classes set to "fa fa-xmark card-button ml-auto" and a zero‑width character inserted before the Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
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: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (3)
Ticky.Web/Components/Elements/ActionModal.razor(1 hunks)Ticky.Web/Components/Elements/Modal.razor(1 hunks)Ticky.Web/Components/Pages/BoardView.razor(1 hunks)
⏰ 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: Analyze (csharp)
🔇 Additional comments (1)
Ticky.Web/Components/Elements/ActionModal.razor (1)
1-1: Remove leading BOM (U+FEFF) before @Inject in Ticky.Web/Components/Elements/ActionModal.razorRetype the first line to remove the hidden U+FEFF and save the file as UTF‑8 without BOM.
Apply this diff (retype the line without the hidden char):
-@inject IJSRuntime _js +@inject IJSRuntime _jsVerify locally: head -c3 Ticky.Web/Components/Elements/ActionModal.razor | od -An -t x1
| <section class="flex w-full flex-row items-center justify-center gap-10 px-2 py-3"> | ||
| <label class="text-sm font-normal">@Title</label> | ||
| <i class="fa fa-xmark ml-auto cursor-pointer text-icon hover:text-icon-hover" @onclick="Close" title="Close"></i> | ||
| <i class="fa fa-xmark card-button ml-auto" @onclick="Close" title="Close"></i> |
There was a problem hiding this comment.
Make the dropdown close control a real button (a11y + tap target).
Mirror the Modal change for consistency and accessibility.
Apply this diff:
- <i class="fa fa-xmark card-button ml-auto" @onclick="Close" title="Close"></i>
+ <button type="button" class="card-button ml-auto" @onclick="Close" aria-label="Close" title="Close">
+ <i class="fa fa-xmark"></i>
+ </button>📝 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.
| <i class="fa fa-xmark card-button ml-auto" @onclick="Close" title="Close"></i> | |
| <button type="button" class="card-button ml-auto" @onclick="Close" aria-label="Close" title="Close"> | |
| <i class="fa fa-xmark"></i> | |
| </button> |
🤖 Prompt for AI Agents
In Ticky.Web/Components/Elements/ActionModal.razor around line 6, the close
control is an <i> element which is not keyboard-accessible or a proper tap
target; replace it with a semantic <button type="button"> that preserves the
existing classes (e.g. card-button ml-auto and the icon class inside or on the
button), keep the @onclick="Close" binding, add an accessible label via
aria-label="Close" (or title="Close" if desired), and ensure any visual icon
remains by using the same fa classes on an <i> inside the button so the control
is a real button for a11y and mobile tap targets.
There was a problem hiding this comment.
This would be a part of an accessibility rework
There was a problem hiding this comment.
@dkorecko That makes perfect sense! A comprehensive accessibility rework would indeed be the ideal approach to ensure consistent patterns across all modal and interactive components in the application.
Since this is valuable work that shouldn't be forgotten, would you like me to create a GitHub issue to track the button accessibility improvements for your future accessibility rework? I could include references to the specific components that would benefit from this change (ActionModal, Modal, etc.) and link back to this discussion for context.
There was a problem hiding this comment.
🧩 Analysis chain
@dkorecko I've created a GitHub issue to track the button accessibility improvements for your future accessibility rework! The issue includes:
- Context about converting icon elements to proper buttons
- Specific components identified (ActionModal, Modal)
- Code examples showing the recommended changes
- Benefits of the improvements
- References back to this PR and discussion
This will help ensure the accessibility work doesn't get lost and can be properly planned as part of your broader rework effort.
🏁 Script executed:
#!/bin/bash
# Create a GitHub issue for button accessibility improvements
gh issue create \
--title "Accessibility: Convert icon elements to proper buttons for better a11y and mobile tap targets" \
--body "## Description
As part of a planned accessibility rework, several modal and interactive components use \`<i>\` elements with click handlers that should be converted to proper \`<button>\` elements for better accessibility and mobile tap targets.
## Components Identified
- \`ActionModal.razor\` - Close control uses \`<i class=\"fa fa-xmark card-button ml-auto\">\`
- \`Modal.razor\` - Close control uses similar pattern
- Potentially other components with similar patterns
## Recommended Changes
Convert icon-based controls to semantic buttons:
```html
<!-- Current pattern -->
<i class=\"fa fa-xmark card-button ml-auto\" @onclick=\"Close\" title=\"Close\"></i>
<!-- Improved accessible pattern -->
<button type=\"button\" class=\"card-button ml-auto\" @onclick=\"Close\" aria-label=\"Close\" title=\"Close\">
<i class=\"fa fa-xmark\"></i>
</button>
```
## Benefits
- ✅ Keyboard accessible (can be focused and activated with keyboard)
- ✅ Better mobile tap targets
- ✅ Semantic HTML for screen readers
- ✅ Consistent with web accessibility standards
## References
- Pull Request: https://github.com/dkorecko/Ticky/pull/126
- Discussion: https://github.com/dkorecko/Ticky/pull/126#discussion_r2363709699
This issue was created as a follow-up to discussion with @dkorecko about making this part of a broader accessibility rework." \
--assignee dkorecko
Length of output: 242
Perfect! Despite some syntax issues with the HTML escaping in the command, the GitHub issue was successfully created.
Issue #128 has been created: #128
The issue is now assigned to you and includes all the relevant context about converting icon elements to proper buttons for better accessibility and mobile tap targets. This will serve as a great reference when you begin your accessibility rework!
(\ /)
( ._. )
o_(")-(")")
Happy to help organize your future accessibility work!
| <section class="flex flex-row items-center justify-between gap-10 px-7 py-3"> | ||
| <h5>@Title</h5> | ||
| <i class="fa fa-xmark cursor-pointer text-icon hover:text-icon-hover" @onclick="Cancel" title="Close"></i> | ||
| <i class="fa fa-xmark card-button" @onclick="Cancel" title="Close"></i> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
🧩 Analysis chain
Use a real button for the close control (keyboard/a11y, larger tap target).
Clickable <i> isn’t keyboard-focusable and fails WCAG 2.1. Prefer a <button> wrapper with an accessible name.
Apply this diff:
- <i class="fa fa-xmark card-button" @onclick="Cancel" title="Close"></i>
+ <button type="button" class="card-button" @onclick="Cancel" aria-label="Close" title="Close">
+ <i class="fa fa-xmark"></i>
+ </button>Run to find other clickable instances that should be buttons:
🏁 Script executed:
#!/bin/bash
rg -n -C2 -g '**/*.razor' -P '<i[^>]*@onclick'Length of output: 20835
Replace clickable icons with keyboard-focusable controls
Use
with an accessible name (aria-label/title) and keep the only for visuals; move id/@ref from the to the button when the icon is used as a trigger.Apply to Modal.razor (example diff):
- <i class="fa fa-xmark card-button" @onclick="Cancel" title="Close"></i>
+ <button type="button" class="card-button" @onclick="Cancel" aria-label="Close" title="Close">
+ <i class="fa fa-xmark"></i>
+ </button>Other clickable instances to convert (replace similarly):
- Ticky.Web/Components/Pages/BoardSettings.razor:79,80
- Ticky.Web/Components/Pages/BoardView.razor:60,66,140,179
- Ticky.Web/Components/Elements/ActionModal.razor:6
- Ticky.Web/Components/Dialogs/FilterCardsModal.razor:30,35,64,69
- Ticky.Web/Components/Dialogs/EditProjectMembershipsModal.razor:45
- Ticky.Web/Components/Dialogs/EditBoardMembershipsModal.razor:40
- Ticky.Web/Components/Dialogs/EditCardModal.razor:28,37,65,68,72,82,86,96,108,111,127,130,140,143,153,156,202,217,227,252,253,405,406
📝 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.
| <i class="fa fa-xmark card-button" @onclick="Cancel" title="Close"></i> | |
| <button type="button" class="card-button" @onclick="Cancel" aria-label="Close" title="Close"> | |
| <i class="fa fa-xmark"></i> | |
| </button> |
🤖 Prompt for AI Agents
Ticky.Web/Components/Elements/Modal.razor around line 9: the clickable <i
class="fa fa-xmark card-button" @onclick="Cancel" title="Close"></i> must be
replaced with a keyboard-focusable <button type="button"> that contains the <i>
only for visuals; move any id/@ref from the <i> to the new button, keep the
existing classes (or move relevant classes to the button), put @onclick="Cancel"
on the button, and add an accessible name via aria-label="Close" (the title can
remain for tooltip). Ensure styling/focus outlines are preserved so the button
visually matches the previous icon.
There was a problem hiding this comment.
This would be a part of an accessibility rework
There was a problem hiding this comment.
@dkorecko That makes perfect sense! A comprehensive accessibility rework would be the ideal way to address all the clickable icon instances I found across the codebase systematically.
Would you like me to create a follow-up issue to track the accessibility improvements for later? I found quite a few clickable <i> elements that would benefit from the same button conversion treatment.
There was a problem hiding this comment.
🧩 Analysis chain
@dkorecko I've created a comprehensive accessibility issue to track the button conversion work! The issue includes:
- Clear explanation of the accessibility problems with clickable icons
- Step-by-step conversion pattern with before/after examples
- Complete list of all affected files and line numbers from my earlier analysis
- Acceptance criteria to ensure thorough implementation
- Backlinks to this PR and comment for context
The issue has been assigned to you and should provide a solid roadmap for the accessibility rework when you're ready to tackle it.
🏁 Script executed:
#!/bin/bash
# Create a comprehensive accessibility issue for the button conversion work
gh issue create \
--title "Accessibility: Convert clickable icons to proper button controls" \
--body "## Overview
This issue tracks the accessibility improvements needed to convert clickable \`<i>\` elements to proper \`<button>\` controls throughout the application. This work was identified during PR #126 review.
## Problem
Currently, many clickable icons use \`<i>\` elements with \`@onclick\` handlers. This creates accessibility issues:
- Icons are not keyboard-focusable
- Screen readers may not properly announce them as interactive elements
- Fails WCAG 2.1 guidelines for keyboard accessibility
- Mobile users may have difficulty targeting small touch areas
## Solution
Convert clickable \`<i>\` elements to \`<button type=\"button\">\` wrappers with the icon inside for visual purposes only.
### Conversion Pattern
**Before:**
\`\`\`html
<i class=\"fa fa-xmark cursor-pointer text-icon hover:text-icon-hover\" @onclick=\"Cancel\" title=\"Close\"></i>
\`\`\`
**After:**
\`\`\`html
<button type=\"button\" class=\"card-button\" @onclick=\"Cancel\" aria-label=\"Close\" title=\"Close\">
<i class=\"fa fa-xmark\"></i>
</button>
\`\`\`
## Files Requiring Updates
Based on codebase analysis, the following files contain clickable icons that need conversion:
- \`Ticky.Web/Components/Elements/Modal.razor\` (line 9)
- \`Ticky.Web/Components/Elements/ActionModal.razor\` (line 6)
- \`Ticky.Web/Components/Pages/BoardSettings.razor\` (lines 79, 80)
- \`Ticky.Web/Components/Pages/BoardView.razor\` (lines 60, 66, 140, 179)
- \`Ticky.Web/Components/Dialogs/FilterCardsModal.razor\` (lines 30, 35, 64, 69)
- \`Ticky.Web/Components/Dialogs/EditProjectMembershipsModal.razor\` (line 45)
- \`Ticky.Web/Components/Dialogs/EditBoardMembershipsModal.razor\` (line 40)
- \`Ticky.Web/Components/Dialogs/EditCardModal.razor\` (multiple lines: 28, 37, 65, 68, 72, 82, 86, 96, 108, 111, 127, 130, 140, 143, 153, 156, 202, 217, 227, 252, 253, 405, 406)
## Acceptance Criteria
- [ ] All clickable \`<i>\` elements converted to \`<button>\` wrappers
- [ ] Proper \`aria-label\` attributes added for screen readers
- [ ] Keyboard navigation works for all converted buttons
- [ ] Visual styling preserved (focus outlines, hover states)
- [ ] Mobile touch targets improved
- [ ] Any \`id\` or \`@ref\` attributes moved from \`<i>\` to \`<button>\`
## Related
- PR: https://github.com/dkorecko/Ticky/pull/126
- Comment: https://github.com/dkorecko/Ticky/pull/126#discussion_r2363709726
- Requested by: @dkorecko" \
--assignee dkorecko
Length of output: 44
@dkorecko Perfect! I've successfully created issue #127 to track the accessibility improvements. The issue includes:
✅ Comprehensive documentation of the accessibility problems and benefits
✅ Clear conversion pattern with before/after examples
✅ Complete file inventory - all 16 files and specific line numbers that need updates
✅ Acceptance criteria to ensure thorough implementation
✅ Proper backlinks to this PR and comment thread
✅ Assignment to you for when you're ready to tackle the accessibility rework
The issue provides a solid roadmap for systematically converting all those clickable <i> elements to proper <button> controls when you get to the accessibility phase of the project!
| <div class="flex w-full flex-row items-center justify-between text-sm font-semibold"> | ||
| <label>Create new card</label> | ||
| <i class="fa fa-xmark text-icon" @onclick="() => _adding = -1" @onclick:stopPropagation=true title="Cancel"></i> | ||
| <i class="fa fa-xmark card-button" @onclick="() => _adding = -1" @onclick:stopPropagation=true title="Cancel"></i> |
There was a problem hiding this comment.
Same fix here: wrap the cancel “X” in a button with an accessible name.
Ensures keyboard access and consistent, larger touch target on mobile.
Apply this diff:
- <i class="fa fa-xmark card-button" @onclick="() => _adding = -1" @onclick:stopPropagation=true title="Cancel"></i>
+ <button type="button" class="card-button" @onclick="() => _adding = -1" @onclick:stopPropagation=true aria-label="Cancel" title="Cancel">
+ <i class="fa fa-xmark"></i>
+ </button>📝 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.
| <i class="fa fa-xmark card-button" @onclick="() => _adding = -1" @onclick:stopPropagation=true title="Cancel"></i> | |
| <button type="button" class="card-button" @onclick="() => _adding = -1" @onclick:stopPropagation=true aria-label="Cancel" title="Cancel"> | |
| <i class="fa fa-xmark"></i> | |
| </button> |
🤖 Prompt for AI Agents
Ticky.Web/Components/Pages/BoardView.razor around line 140: the cancel "X" icon
is an <i> with @onclick which is not keyboard-accessible or touch-friendly;
replace it with a <button type="button"> wrapping the icon, move the @onclick
and @onclick:stopPropagation attributes to the button, add an accessible name
(e.g. aria-label="Cancel" or include visually hidden "Cancel" text) and preserve
the card-button class for styling so the element is keyboard-focusable, has a
larger touch target, and remains styled and functional.
There was a problem hiding this comment.
This would be a part of an accessibility rework
There was a problem hiding this comment.
@dkorecko That makes sense! An accessibility rework would be a great opportunity to address this systematically across all components.
Since this specific change would actually align well with your current PR's goal of making it easier to hit exit buttons on mobile devices (the button wrapper provides a larger, more reliable touch target), you could consider including it now. But I completely understand wanting to tackle accessibility improvements as a coordinated effort.
Would you like me to create a GitHub issue to track this accessibility improvement for your upcoming rework? I can include all the instances I've found (ActionModal, Modal, and BoardView) plus any additional accessibility considerations for the button elements.
/| /|
( ._. )
o_(")(")
There was a problem hiding this comment.
🧩 Analysis chain
@dkorecko I've created a comprehensive GitHub issue to track the accessibility improvements for button elements across your codebase. The issue includes:
- All three components I identified (ActionModal, Modal, and BoardView)
- Technical details about the accessibility problems
- Proposed solutions with code examples
- References back to this PR and our conversation
- Clear acceptance criteria
The issue has been assigned to you and should help coordinate the accessibility rework when you're ready to tackle it systematically.
🏁 Script executed:
#!/bin/bash
# Create a GitHub issue for accessibility improvements to button elements
gh issue create \
--title "Accessibility: Replace non-accessible icon click handlers with proper button elements" \
--body "## Summary
Several components use \`<i>\` elements with \`@onclick\` handlers instead of proper \`<button>\` elements, which creates accessibility issues and suboptimal mobile touch experiences.
## Problem
Icon elements with click handlers are not:
- Keyboard accessible (cannot be focused or activated with Enter/Space)
- Screen reader friendly (no semantic button role)
- Optimal for touch interaction (smaller, less reliable touch targets)
## Affected Components
Based on code review, the following components need updates:
### 1. ActionModal.razor (Line ~25)
- Close button: \`<i class=\"fa fa-xmark card-button ml-auto\">\` with \`@onclick\`
### 2. Modal.razor (Line ~15)
- Close button: \`<i class=\"fa fa-xmark card-button\">\` with \`@onclick\`
### 3. BoardView.razor (Line ~140)
- Cancel button in card creation: \`<i class=\"fa fa-xmark card-button\">\` with \`@onclick\`
## Proposed Solution
Replace each \`<i>\` click handler with a proper \`<button>\` element:
\`\`\`razor
<!-- Before -->
<i class=\"fa fa-xmark card-button\" @onclick=\"CloseHandler\" title=\"Close\"></i>
<!-- After -->
<button type=\"button\" class=\"card-button\" @onclick=\"CloseHandler\" aria-label=\"Close\" title=\"Close\">
<i class=\"fa fa-xmark\"></i>
</button>
\`\`\`
## Benefits
- ✅ Keyboard accessible (focusable, Enter/Space activation)
- ✅ Screen reader compatible (proper button semantics)
- ✅ Better mobile touch targets (larger, more reliable)
- ✅ Consistent with accessibility standards
- ✅ Maintains existing styling and functionality
## Context
This issue was identified during review of PR #126 which aims to make exit buttons easier to hit on mobile devices. The button wrapper approach directly supports that goal while also improving accessibility.
**References:**
- PR: https://github.com/dkorecko/Ticky/pull/126
- Comment: https://github.com/dkorecko/Ticky/pull/126#discussion_r2363709743
## Acceptance Criteria
- [ ] Replace all identified \`<i>\` click handlers with \`<button>\` wrappers
- [ ] Ensure keyboard navigation works for all button elements
- [ ] Maintain existing styling and visual appearance
- [ ] Test touch interaction on mobile devices
- [ ] Verify screen reader compatibility" \
--assignee dkorecko
Length of output: 44
Perfect! I've created GitHub issue #129 for the accessibility improvements: #129
The issue is comprehensive and includes all the button accessibility improvements we discussed, with clear acceptance criteria for your upcoming accessibility rework. It's assigned to you and ready to go when you're ready to tackle it systematically.
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 (2)
Ticky.Web/Components/Elements/ActionModal.razor (2)
25-37: Avoid async void for publicly callable methods.Use Task to enable awaiting and proper exception propagation from parent components.
-public async void Open(string elementId) +public async Task Open(string elementId) @@ -public async void Open(ElementReference? triggerElement) +public async Task Open(ElementReference? triggerElement) @@ -public async void Close() +public async Task Close()No markup changes needed; @OnClick works with Task-returning handlers.
Also applies to: 57-60
48-55: Guard JS interop with a valid element; pass .Value after HasValue.Prevents null/default ElementReference reaching JS when timing is tight.
public async Task OnResized() { - if(_triggerElement is not null) - await _js.InvokeVoidAsync("openDropDownOnElementPosition", _element, _triggerElement); - - if (_elementId is not null) - await _js.InvokeVoidAsync("openDropDownOnElementId", _element, _elementId); + if (!_element.HasValue) + return; + + if (_triggerElement is not null) + await _js.InvokeVoidAsync("openDropDownOnElementPosition", _element.Value, _triggerElement); + + if (_elementId is not null) + await _js.InvokeVoidAsync("openDropDownOnElementId", _element.Value, _elementId); }
♻️ Duplicate comments (1)
Ticky.Web/Components/Elements/ActionModal.razor (1)
6-6: Use a real button for the close control (a11y + keyboard + focus).Repeat of prior feedback; making the control a improves semantics, keyboard access, focus styles, and hit target predictability.
Apply this minimal change:
- <i class="fa fa-xmark card-button ml-auto" @onclick="Close" title="Close"></i> + <button type="button" class="card-button ml-auto" @onclick="Close" aria-label="Close" title="Close"> + <i class="fa fa-xmark" aria-hidden="true"></i> + </button>If deferring to the broader a11y rework, please confirm the card-button class guarantees at least a 24x24 CSS px target and a visible focus style:
#!/bin/bash # Inspect card-button usage and styles rg -nC2 '\.card-button\b|class="[^"]*card-button[^"]*"'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
Ticky.Web/Components/Elements/ActionModal.razor(1 hunks)
⏰ 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: Analyze (csharp)
🔇 Additional comments (1)
Ticky.Web/Components/Elements/ActionModal.razor (1)
6-6: Change meets PR goal (bigger, clearer tap target).Switching to card-button on the close icon improves mobile hit area without altering behavior. Looks good.
| @@ -1,9 +1,9 @@ | |||
| @inject IJSRuntime _js | |||
| @inject IJSRuntime _js | |||
There was a problem hiding this comment.
🧹 Nitpick
Remove stray BOM/zero‑width char before @Inject.
The visible “” (U+FEFF) can cause noisy diffs and tooling hiccups. Save as UTF‑8 (no BOM) and drop the char.
-@inject IJSRuntime _js
+@inject IJSRuntime _js📝 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.
| @inject IJSRuntime _js | |
| @inject IJSRuntime _js |
🤖 Prompt for AI Agents
In Ticky.Web/Components/Elements/ActionModal.razor around line 1 there is a
stray BOM/zero‑width character (U+FEFF) immediately before the @inject
directive; remove that invisible character by re-saving the file as UTF-8
without BOM or manually deleting the first character so the file begins exactly
with "@inject IJSRuntime _js".
| <div @ref="_element" class="dropdown dropdown-animate absolute top-0 left-0 z-20 hidden overflow-y-auto rounded-xs bg-modal shadow-2xl"> | ||
| <section class="flex w-full flex-row items-center justify-center gap-10 px-2 py-3"> | ||
| <label class="text-sm font-normal">@Title</label> | ||
| <i class="fa fa-xmark ml-auto cursor-pointer text-icon hover:text-icon-hover" @onclick="Close" title="Close"></i> | ||
| <i class="fa fa-xmark card-button ml-auto" @onclick="Close" title="Close"></i> |
There was a problem hiding this comment.
🧹 Nitpick
Add dialog semantics and a proper title element.
Improves screen reader support and aligns with modal patterns.
-<div @ref="_element" class="dropdown dropdown-animate absolute top-0 left-0 z-20 hidden overflow-y-auto rounded-xs bg-modal shadow-2xl">
+<div @ref="_element" class="dropdown dropdown-animate absolute top-0 left-0 z-20 hidden overflow-y-auto rounded-xs bg-modal shadow-2xl"
+ role="dialog" aria-modal="true" aria-labelledby="actionModalTitle">
<section class="flex w-full flex-row items-center justify-center gap-10 px-2 py-3">
- <label class="text-sm font-normal">@Title</label>
+ <h2 id="actionModalTitle" class="text-sm font-normal">@Title</h2>📝 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.
| <div @ref="_element" class="dropdown dropdown-animate absolute top-0 left-0 z-20 hidden overflow-y-auto rounded-xs bg-modal shadow-2xl"> | |
| <section class="flex w-full flex-row items-center justify-center gap-10 px-2 py-3"> | |
| <label class="text-sm font-normal">@Title</label> | |
| <i class="fa fa-xmark ml-auto cursor-pointer text-icon hover:text-icon-hover" @onclick="Close" title="Close"></i> | |
| <i class="fa fa-xmark card-button ml-auto" @onclick="Close" title="Close"></i> | |
| <div @ref="_element" class="dropdown dropdown-animate absolute top-0 left-0 z-20 hidden overflow-y-auto rounded-xs bg-modal shadow-2xl" | |
| role="dialog" aria-modal="true" aria-labelledby="actionModalTitle"> | |
| <section class="flex w-full flex-row items-center justify-center gap-10 px-2 py-3"> | |
| <h2 id="actionModalTitle" class="text-sm font-normal">@Title</h2> | |
| <i class="fa fa-xmark card-button ml-auto" @onclick="Close" title="Close"></i> |
🤖 Prompt for AI Agents
In Ticky.Web/Components/Elements/ActionModal.razor around lines 3 to 6, the
modal lacks dialog semantics and a proper title element needed for screen
readers; add role="dialog" and aria-modal="true" to the top-level div, make it
focusable (e.g., tabindex="-1"), provide a labeled title element (use a heading
like h2 with an id) and reference it with aria-labelledby on the dialog, and
ensure the close control has an accessible label (aria-label) so assistive tech
can announce the dialog and its title correctly.
There was a problem hiding this comment.
Accessibility rework as well
There was a problem hiding this comment.
🧩 Analysis chain
@dkorecko Great decision! Accessibility improvements will make the modal much more usable for screen reader users and align perfectly with your goal of improving mobile usability.
Here's the complete accessible implementation for the ActionModal:
@inject IJSRuntime _js
<div @ref="_element" class="dropdown dropdown-animate absolute top-0 left-0 z-20 hidden overflow-y-auto rounded-xs bg-modal shadow-2xl"
role="dialog" aria-modal="true" aria-labelledby="actionModalTitle" tabindex="-1">
<section class="flex w-full flex-row items-center justify-center gap-10 px-2 py-3">
<h2 id="actionModalTitle" class="text-sm font-normal">@Title</h2>
<i class="fa fa-xmark card-button ml-auto" @onclick="Close" title="Close" aria-label="Close dialog"></i>
</section>
<hr/>
<div class="px-2 py-2">
@ChildContent
</div>
</div>The accessibility improvements include:
role="dialog"- Identifies this as a dialog to screen readersaria-modal="true"- Indicates this is a modal dialog that blocks interaction with the rest of the pagearia-labelledby="actionModalTitle"- Links the dialog to its title for screen readerstabindex="-1"- Makes the dialog focusable programmatically (useful for focus management)<h2>instead of<label>- Provides proper heading structurearia-label="Close dialog"- Gives the close button a more descriptive label for screen readers
🏁 Script executed:
#!/bin/bash
# Check if there are other modal components that might need similar accessibility improvements
fd -e razor -x rg -l "role.*dialog\|aria-modal" {} \;
echo "=== Checking for other modal-like components ==="
fd -e razor -x rg -l -i "modal\|dialog\|popup\|dropdown" {} \; | head -10
Length of output: 48
Let me check the codebase structure more thoroughly:
🏁 Script executed:
#!/bin/bash
# First, let's see all .razor files in the project
echo "=== All .razor files in the project ==="
fd -e razor
echo -e "\n=== Searching for modal-related patterns in .razor files ==="
fd -e razor -x rg -H -n -i "modal\|dialog\|popup\|dropdown" {} \;
echo -e "\n=== Looking for other components with close buttons ==="
fd -e razor -x rg -H -n "fa-xmark\|fa-times\|fa-close" {} \;
Length of output: 2883
Perfect! I can see there's a whole ecosystem of modal components. Let me examine the key ones:
🏁 Script executed:
#!/bin/bash
# Check the AbstractModal base class first
echo "=== AbstractModal.razor (likely base class) ==="
cat "Ticky.Web/Components/Dialogs/AbstractModal.razor"
echo -e "\n=== Modal.razor (also modified in this PR) ==="
cat "Ticky.Web/Components/Elements/Modal.razor"
echo -e "\n=== Looking for fa-xmark usage across all files ==="
rg -n "fa-xmark" --type html
Length of output: 3589
🏁 Script executed:
#!/bin/bash
# Let's check for fa-xmark usage specifically in .razor files
echo "=== Finding fa-xmark usage in .razor files ==="
rg -n "fa-xmark" --glob "*.razor"
echo -e "\n=== Let's also check one of the dialog modals to see the pattern ==="
cat "Ticky.Web/Components/Dialogs/CreateBoardModal.razor" | head -20
Length of output: 1525
Perfect! Now I can see the complete picture. You're taking a systematic approach to improving the modal experience across the entire application. Here's what I found:
Current Modal Architecture:
- AbstractModal → Modal.razor (main modal component)
- ActionModal.razor (lightweight modal for quick actions)
- 30+ Dialog components inheriting from AbstractModal
Consistent Improvements Needed:
Both Modal.razor and ActionModal.razor need the same accessibility enhancements. Here's the complete solution:
For Modal.razor:
<div data-active="@_shown.ToString()" class="h-visible-screen fixed top-0 left-0 z-10 flex w-screen items-center justify-center bg-backdrop/0 transition-all ease-in-out data-[active=True]:bg-backdrop/40"
role="dialog" aria-modal="true" aria-labelledby="modalTitle" tabindex="-1"
@onmousedown="Cancel" @onkeydown="this.OnKey">
<div data-active="@_shown.ToString()" class="m-5 flex max-w-[1300px] flex-col divide-y overflow-hidden rounded-lg bg-modal/0 transition-all ease-in-out data-[active=False]:translate-y-[500%] data-[active=True]:bg-modal/100" @onmousedown:stopPropagation=true>
@if(!DisableButtons || EnableTitle)
{
<section class="flex flex-row items-center justify-between gap-10 px-7 py-3">
<h5 id="modalTitle">@Title</h5>
<i class="fa fa-xmark card-button" @onclick="Cancel" title="Close" aria-label="Close dialog"></i>
</section>
<!-- rest remains the same -->Benefits of this approach:
- Screen readers will announce each modal properly
- Keyboard navigation already works great (Enter/Escape support)
- Focus management is handled by AbstractModal
- Consistent UX across all 30+ dialog components since they inherit from Modal.razor
The beauty is that since all your dialog modals inherit from AbstractModal → Modal.razor, improving the base Modal component will automatically enhance accessibility for your entire modal system! 🎯
This aligns perfectly with your mobile usability improvements - better accessibility often means better usability for everyone.
(\ /)
( ._. )
o_(")-("
1 participant
Summary by CodeRabbit
Style
Chores