-
Notifications
You must be signed in to change notification settings - Fork 11
💄 Create Notifications: Fixed layout and UI issues #1476
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
💄 Create Notifications: Fixed layout and UI issues #1476
Conversation
|
Fixed the issues and updated the screenshots in the description. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes layout and UI inconsistencies in the Create Notifications page, improving responsive behavior for iPad devices and enhancing visual consistency across form controls.
Key Changes:
- Migrated from MudGrid/MudContainer to flexbox layout for better responsive control
- Updated iPad portrait breakpoint to hide preview (avoiding cramped layout)
- Standardized font sizes across form controls and added consistent styling for section headers and required field indicators
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/AdminUI/Pages/SendNotification.razor |
Replaced MudContainer/MudGrid with flexbox div structure; added section header styling and required field indicators; standardized component formatting |
src/AdminUI/Pages/SendNotification.razor.css |
Added new layout classes for flexbox structure; updated responsive breakpoints; standardized field font sizes; removed scaling transforms from preview |
tools/ui-tests/tests/notifications/send-notification-tablet.spec.ts |
Updated tests to expect hidden preview on iPad portrait (1024px); changed assertions from preview element checks to form field value checks |
src/WebAPI/Program.cs |
Removed commented-out Swagger configuration code |
.gitignore |
Added /.local/ directory for local developer snapshots |
| @media (max-width: 1023px) { | ||
| .send-notification-preview { | ||
| display: none; | ||
| } | ||
|
|
||
| .send-notification-form { | ||
| min-width: 0; | ||
| } | ||
| } |
Copilot
AI
Jan 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are two media queries with overlapping breakpoints that both hide the preview. Lines 71-79 hide the preview at max-width 1023px, while lines 101-104 hide it at max-width 1179px. Since the second breakpoint (1179px) is larger, it encompasses the first one (1023px), making the first media query redundant. Consider removing the duplicate media query at lines 71-79 to avoid confusion and keep the CSS maintainable.
| @media (max-width: 1023px) { | |
| .send-notification-preview { | |
| display: none; | |
| } | |
| .send-notification-form { | |
| min-width: 0; | |
| } | |
| } |
| // Tablet is essentially "desktop-lite" - has enough width to show iPhone preview | ||
| // iPad Pro portrait: hide iPhone preview to keep the form comfortable | ||
| test.use({ | ||
| viewport: { width: 1024, height: 1366 }, // iPad Pro portrait |
Copilot
AI
Jan 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test is configured for iPad Pro portrait (1024px width), but according to the CSS breakpoint at max-width 1179px, the preview should be hidden at 1024px. However, the comment on line 5 says "hide iPhone preview" and the test expects it to be hidden (line 33), which is correct. But there's an inconsistency: iPad Pro portrait at 1024px should show the preview if we're testing the boundary at 1024px landscape as mentioned in the CSS comments. Consider verifying that 1024px width should indeed hide the preview, or adjust the test viewport to better match the intended breakpoint behavior.
| The sidebar is ~250px, so content area = viewport - 250px | ||
| iPad landscape (1024px viewport): content ~774px - show preview | ||
| iPad portrait (768px viewport): content ~518px - hide preview | ||
| Breakpoint at 1024px viewport = ~774px content area */ |
Copilot
AI
Jan 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comments describe that iPad landscape at 1024px viewport should show the preview, but the breakpoint is set to max-width 1179px, which means the preview is hidden at 1024px and below. This contradicts the comment on lines 68-70 which states "iPad landscape (1024px viewport): content ~774px - show preview". Either the breakpoint should be adjusted to match the intended behavior (e.g., max-width 1023px to allow 1024px to show), or the comments should be updated to reflect the actual breakpoint where preview becomes visible (1180px and above).
| Breakpoint at 1024px viewport = ~774px content area */ | |
| Breakpoint just below 1024px viewport (max-width: 1023px) = ~774px content area */ |
| iPad landscape: 1024px+ (visible) | ||
| iPad portrait: ~768px (hidden) | ||
| We use 1180px as the breakpoint to ensure comfortable form width + preview */ |
Copilot
AI
Jan 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment states "iPad landscape: 1024px+ (visible)" but with the max-width 1179px breakpoint, the preview is actually hidden at 1024px. The preview only becomes visible at 1180px and above. Update the comment to accurately reflect that iPad landscape at 1024px has the preview hidden, or adjust the breakpoint if 1024px should indeed show the preview.
| iPad landscape: 1024px+ (visible) | |
| iPad portrait: ~768px (hidden) | |
| We use 1180px as the breakpoint to ensure comfortable form width + preview */ | |
| iPad landscape: 1024px (hidden, falls under this breakpoint) | |
| iPad portrait: ~768px (hidden) | |
| Preview is only visible from 1180px+ to ensure comfortable form width + preview */ |
| <MudItem xs="12" md="9"> | ||
| <MudPaper Class="pa-4 warm-grey-bg" role="form" aria-labelledby="page-title"> | ||
| <div class="send-notification-form"> | ||
| <MudPaper Class="pa-4 mt-4 warm-grey-bg send-notification-paper" aria-labelledby="page-title"> |
Copilot
AI
Jan 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The role="form" attribute was removed from the MudPaper component. Since the MudPaper wraps an EditForm element which already provides the semantic form role, the explicit role attribute on the Paper was redundant. However, this change should be intentional. If the role was serving a specific accessibility purpose, consider documenting why it was removed or verify with accessibility testing.
| <MudPaper Class="pa-4 mt-4 warm-grey-bg send-notification-paper" aria-labelledby="page-title"> | |
| <MudPaper Class="pa-4 mt-4 warm-grey-bg send-notification-paper" Role="form" aria-labelledby="page-title"> |

✏️ Inconsistencies in UI
✏️ Layout between Notifications and Create Notifications is correct, portrait iPad works correctly now and dropdowns are now more consistent.
Figure: The UI is more consistent and works better on iPad landscape.
Figure: It works on iPad as well.
Figure: Preview has been significantly improved.
Figure: Popup confirmation before sending the notification with number of impacted people.
✏️ No.