feat(ui): add bulk edit notifications modal and floating action bar to uptime monitors#3448
Conversation
ashu130698
requested review from
Br0wnHammer,
Owaiseimdad,
ajhollid and
karenvicent
as code owners
|
LGTM UI wise. Can you make sure those margins are the same? (the ones on the right are the right margins): 
|
ajhollid
left a comment
ajhollid
left a comment
There was a problem hiding this comment.
Visually looks great 👍
There are some UX issues that need to be resolved, and most importantly selection is not cleared on page change.
Please see my comments in the code review and revise as needed, thanks!
| left: "50%", | ||
| transform: "translateX(-50%)", | ||
| zIndex: theme.zIndex.snackbar, | ||
| padding: theme.spacing(1.5, 3), |
There was a problem hiding this comment.
Please use the Utils/Theme/constants util for all spacing values
| const handleBulkEditSuccess = () => { | ||
| setIsBulkEditModalOpen(false); | ||
| setSelectedMonitors([]); | ||
| refetch(); |
There was a problem hiding this comment.
What about failure? Modal stays open on failure right now.
There was a problem hiding this comment.
This hasn't been addressed yet
| @@ -1,5 +1,6 @@ | |||
| import Stack from "@mui/material/Stack"; | |||
There was a problem hiding this comment.
What about all the other monitor pages? Currently only uptime monitors are bulk editable 🤔
EDIT
I see you addressed this in your PR statement, noted 👍
| }; | ||
|
|
||
| const handleClearSelection = () => { | ||
| setSelectedMonitors([]); |
There was a problem hiding this comment.
This should be called on pagination change too
| onClick={() => setIsBulkEditModalOpen(true)} | ||
| > | ||
| {t("pages.common.monitors.bulkEdit.editButton", { | ||
| defaultValue: "Edit Notifications", |
There was a problem hiding this comment.
No need for defaultValue, please remove all these
| <Checkbox | ||
| checked={monitors.length > 0 && selectedMonitors.length === monitors.length} | ||
| indeterminate={ | ||
| selectedMonitors.length > 0 && selectedMonitors.length < monitors.length | ||
| } | ||
| onChange={(e) => { | ||
| if (e.target.checked) { | ||
| onSelectAll(monitors.map((m) => m.id)); | ||
| } else { | ||
| onSelectAll([]); | ||
| } | ||
| }} | ||
| onClick={(e) => e.stopPropagation()} | ||
| /> | ||
| ), | ||
| render: (row) => { | ||
| return ( | ||
| <Checkbox | ||
| checked={selectedMonitors.includes(row.id)} | ||
| onChange={() => onSelectMonitor(row.id)} | ||
| onClick={(e) => e.stopPropagation()} | ||
| /> | ||
| ); | ||
| }, | ||
| }, |
There was a problem hiding this comment.
This needs to be adjusted to account for small screen card view:
Please see the table class to see how the cards work on small screens.
|
@ajhollid @gorkem-bwl Thank you so much for the detailed review and catching those quirks! I have just pushed an updated commit addressing all of your feedback. Here is a breakdown of the fixes:
|
ajhollid
left a comment
ajhollid
left a comment
There was a problem hiding this comment.
There's an outstanding item from the last review, and two other issues I've noticed
The floating action button doesn't close when the modal opens
and
There doesn't appear to be a "select all" option on mobile, and the checkbox for individual cards should be left aligned, not center aligned.
| const handleBulkEditSuccess = () => { | ||
| setIsBulkEditModalOpen(false); | ||
| setSelectedMonitors([]); | ||
| refetch(); |
There was a problem hiding this comment.
This hasn't been addressed yet
|
Thanks for the thorough review @ajhollid! I've pushed a new commit that addresses all three issues: Floating Action Bar: It now properly hides behind a !isBulkEditModalOpen condition so it doesn't stay awkwardly visible when the modal mounts. Mobile Layout: I updated the underlying component so that an empty header label allows the row content (like our checkbox) to cleanly expand to size=12 and naturally left-align. I also added a missing "Select All" option explicitly for the mobile card view!Modal Error Handling: To address the poor UX of the modal staying open on an API failure, I updated the logic so it now always closes the modal upon API completion (whether it succeeds or errors). This way, the user relies on the global error toast for the failure feedback, but their underlying checkbox selection is safely preserved so they can easily click "Edit" and try again! (Just let me know if you actually preferred the modal to stay open on failure while just handling the error state completely differently—I can easily swap that back if I misunderstood that piece of feedback!) |
|
Great catch on the mobile view overflow! The Floating Action Bar's slide animation was unintentionally stripping our CSS transform centering on small screens. I've switched the layout to use auto-margins and pushed the fix—it now centers perfectly on mobile. The PR should be fully good to go now! |
ajhollid
left a comment
ajhollid
left a comment
There was a problem hiding this comment.
Unfortunately, the changes don't seem to solve the issue. In fact it seems to be broken in both mobile and desktop now:
the FAB should be centered in the screen.
|
Thanks for catching this @ajhollid! You're absolutely right - the FAB should be I see the issue now - the margin-based centering approach isn't working reliably Could you please verify that the latest commit fixes the alignment issue on your |
| @@ -1,5 +1,6 @@ | |||
| { | |||
| "common": { | |||
| "selected": "selected", | |||
There was a problem hiding this comment.
This is not the correct structure for a key. Since it is used in a button, let's put it in common.buttons
These strings need to be logically organized or this file becomes unmanageable very quickly.
| variant="body2" | ||
| color="text.secondary" | ||
| > | ||
| {t("common.selectAll", "Select All")} |
There was a problem hiding this comment.
This string does not exist in the translation file:
i18next::translator: missingKey en translation common.selectAll Select All
and if it did it is not in the logically correct location. It should belong to the common monitor strings object, probably as an option in common.monitors.bulkEdit
|
Thanks again @ajhollid! You were totally right—the translation file needed to be properly structured before it got unmanageable. I've successfully migrated the selected key into common.buttons.selected and added the missing selectAll string exactly where you requested in pages.common.monitors.bulkEdit. I also updated all the React components to properly fetch from this new nested structure. Everything should be tightly organized and good to go now! Thanks for taking the time to catch these. |
|
@ajhollid - Just wanted to follow up! I've implemented all your suggestions. Would appreciate another review when you get a moment. Thanks! |
There was a problem hiding this comment.
Hey @ashu130698 ,
Apologies for the long delay here, got wrapped up in other issues.
I see some casts that I missed in previous reviews, lets remove those to preserve type safety.
There's also some hardcoded dimensional values that need to reference the LAYOUT or SPACING constants, please update those 👍
Thanks again for your work here!
|
|
||
| <RadioGroup | ||
| value={action} | ||
| onChange={(e) => setAction(e.target.value as "add" | "remove" | "set")} |
There was a problem hiding this comment.
Let's remove this cast, we should never be casting values, we lose type safety when we do so
| <Stack | ||
| spacing={theme.spacing(4)} | ||
| mt={theme.spacing(2)} |
There was a problem hiding this comment.
These should use the LAYOUT and SPACING constants in the Utils dir, see other stacks for examples
| onComplete(!!res); | ||
| }; | ||
|
|
||
| // Prevent submitting empty arrays unless the action is 'set' (as per PR feedback!) |
There was a problem hiding this comment.
Let's remove the "as per PR Feedback!" from this comment. There's no reference to which PR or what the feedback was, so it doesn't add anything useful here.
|
There should be a few fixes to this. We need to use our components for select boxes, not MUI, and there is a glitch in the 2nd column (eg. column is centered). The modal is a bit packed but we can fix it after the PR 
Can you work on those @harsh-aghara ? |
|
Hey @gorkem-bwl I am on it! |
|
@ajhollid I've pushed an update addressing your recent feedback! I removed all the unsafe type casts in favor of strict type guards, replaced the hardcoded dimensional values with the LAYOUT and SPACING theme constants, and cleaned up the comments as requested. @gorkem-bwl Sounds like a great plan. I will put this branch on hold for now. Once #3626 is merged, I will rebase and integrate my edit notifications modal directly into the new modular BulkActionsBar! |
ashu130698
force-pushed
the
feat/frontend-bulk-notifications
branch
2 times, most recently
from
f3a9e41 to
9027707
Compare
|
I have successfully rebased this PR onto the latest upstream/develop. Key updates in this rebase:
|
|
Hi @ashu130698 , Pleaes review the modal on mobile, it does not fit currently 
The check icon when a single monitor is selected is the wrong size as well 
Please revert the change to the Table component as well (text.primary -> text.secondary), this is out of scope for this PR. Additionally, all colors should be referenced as Please remove all inline default translations as well, there should be no defaults provided. Thanks! |
ashu130698
force-pushed
the
feat/frontend-bulk-notifications
branch
from
9027707 to
c5e6637
Compare
|
@ajhollid Thanks for the thorough review! I've addressed all of your feedback and pushed an updated version. Here is what was fixed: Mobile Layout: Added responsive max-width boundaries and flexWrap to the BulkActionsBar so it no longer overflows off the screen on mobile devices. Let me know if there's anything else you need before we merge this in! |
ajhollid
left a comment
ajhollid
left a comment
There was a problem hiding this comment.
The floating action bar is not dismissed when the modal opens, and the modal controls are blocked
The floating action bar is centered in the content window, but it should really be centered, offset by the width of the sidebar so it appears centered with respect to the viewport
There are a number of other issues that are either new or that I missed in previous reviews, so please have a look at those.
Another dev has already implemented the intermediate checkbox solution, so please merge that in from develop and use that 👍 Thanks for your continued work on this!
| alignItems: "center", | ||
| gap: LAYOUT.XS, | ||
| boxShadow: theme.shadows[8], | ||
| maxWidth: isSmall ? "calc(100vw - 32px)" : "none", |
There was a problem hiding this comment.
What is this calculation? Where did 32px come from? There should be no magic numbers
| open={open} | ||
| title={t("pages.common.monitors.bulkEdit.title")} | ||
| onCancel={onClose} | ||
| onConfirm={isMissingSelection ? undefined : handleConfirm} |
There was a problem hiding this comment.
This is kind of a hack. The button should be disabled, really, and not just omitted entirely by a side effect
| <Select | ||
| multiple | ||
| value={notificationIds} | ||
| onChange={(e) => { | ||
| const val = e.target.value; | ||
| if (typeof val === "string") { | ||
| setNotificationIds(val.split(",")); | ||
| } else if (Array.isArray(val)) { | ||
| setNotificationIds(val.filter((v) => typeof v === "string")); | ||
| } | ||
| }} | ||
| fieldLabel={t("pages.common.monitors.bulkEdit.selectLabel")} | ||
| placeholder={t("pages.common.monitors.bulkEdit.selectPlaceholder")} | ||
| > | ||
| {notifications?.map((n) => ( | ||
| <MenuItem | ||
| key={n.id} | ||
| value={n.id} | ||
| > | ||
| {n.notificationName} ({n.type.toUpperCase()}) | ||
| </MenuItem> | ||
| ))} | ||
| </Select> |
There was a problem hiding this comment.
This displays the internal MongoID of the selected notification, which is not useful to the user. Probably better to use the Autocomplete component here instead of select
| import Checkbox from "@mui/material/Checkbox"; | ||
| import type { CheckboxProps } from "@mui/material/Checkbox"; | ||
| import { Square, SquareCheck } from "lucide-react"; | ||
| import { Square, SquareCheck, MinusSquare } from "lucide-react"; |
There was a problem hiding this comment.
This has already been implemented by another PR, please merge that one and use that implementation, thanks!
|
Please take care of the conflicts. |
5 participants
Changes
This PR implements the frontend user interface for bulk-editing monitor notifications, building seamlessly on the backend work from my previous PR (#3376).
As requested by @gorkem-bwl in the issue thread, this introduces a clean, non-intrusive Floating Action Bar pattern that appears only when multiple monitors are selected in the table, rather than a bulky static "bulk edit" button.
Core components introduced:
add,remove,set) and query your available notification channels via useGet.selectedMonitorsarray into the Uptime/Monitors/index.tsx page to sync the bar and table.(Note: Currently applied to the Uptime Monitors page as an MVP to gather feedback on the design. If approved, I will follow up and apply this exact same logic to the Infrastructure and PageSpeed tables!)
Fixes #2320
npm run formatin server and client directories, which automatically formats your code.