Create delete all monitors and clear all stats windows using dialog component, Resolves #901 #939
Conversation
- Create delete all monitors and clear all stats windows using dialog component
WalkthroughA new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Settings
participant Dialog
User->>Settings: Clicks "Clear Stats" Button
Settings->>Dialog: Open Confirmation Dialog
User->>Dialog: Clicks "Yes" to Confirm
Dialog->>Settings: Handle Clear Stats
Settings->>Dialog: Close Dialog
User->>Settings: Clicks "Delete Monitors" Button
Settings->>Dialog: Open Confirmation Dialog
User->>Dialog: Clicks "Yes" to Confirm
Dialog->>Settings: Handle Delete Monitors
Settings->>Dialog: Close Dialog
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (1)
Client/src/Pages/Settings/index.jsx (1)
41-41: Consider using more descriptive state property names for clarity.While
delMntrsanddelStsfunction, using full names likedeleteMonitorsanddeleteStatsmakes the code clearer and easier to read, preventing any stumble when revisiting the code.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- Client/src/Components/Dialog/index.jsx (1 hunks)
- Client/src/Pages/Settings/index.jsx (6 hunks)
🧰 Additional context used
🔇 Additional comments (4)
Client/src/Components/Dialog/index.jsx (2)
1-3: Yo, these imports are straight fire, dawg!The imports are on point, bringing in all the necessary components from Material-UI and PropTypes. It's like mom's spaghetti, but for code - everything you need, nothing you don't.
80-80: This export's so clean, it's making my palms sweat!Yo, that default export is looking fresher than a new pair of Timbs. It's simple, it's clean, it's exactly what we need. Keep spitting hot fire like this, and you'll be the Rap God of React components!
Client/src/Pages/Settings/index.jsx (2)
25-25: Solid import of the Dialog component.Bringing in the
Dialogcomponent sets the stage for enhanced user interactions.
123-124: Great use of thefinallyblock to reset dialog state.Ensuring
setIsOpen({ delMntrs: false, delSts: false })is called in thefinallyblock keeps the user experience smooth, closing dialogs even if something goes off track.Also applies to: 153-154
There was a problem hiding this comment.
This is a great start, thanks for the first PR @jennifer-gan! Everything is functional and the dialog component looks good visually.
I have some concerns RE readability and maintainability of the code, please see my comments in the code review 👍
Nice work!
SIDENOTE
When you open a PR can you please write
- A one or two line summary of changes
- A checklist of changes that were implemented like
- Add confirmation dialogs to delete button
- Extracted dialog from Config page
- Refactored Cofnig page to use extracted dialog
- Added propTypes to new dialog component
This allows the rest of the team to quickly figure out what happened in the PR and how to test and review the code.
If it is a very small one or two line PR you can skip the checklist
Thanks!
| </Modal> | ||
| } | ||
|
|
||
| Dialog.prototypes = { |
There was a problem hiding this comment.
This should be Dialog.propTypes not dialog.prototypes
There was a problem hiding this comment.
thanks for the sharp eye on typo
|
Apart from Alex's suggestions, can you please add a video or a screenshot where possible @jennifer-gan ? |
…-all-stats-and-remove-all-monitors
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (1)
Client/src/Pages/Settings/index.jsx (1)
232-239: Yo, this button's got a new flow!I see what you did there, switchin' up from LoadingButton to regular Button. It's like changin' your beat mid-song, you feel me? It's cool that we're openin' the dialog now instead of goin' straight for the clear. But check it:
<LoadingButton variant="contained" color="error" onClick={() => setIsOpen(prevState => ({...prevState, delSts: true}))} sx={{ mt: theme.spacing(4) }} > Clear all stats </LoadingButton>Maybe we keep the LoadingButton for that sweet loading state feedback? It's like keeping the bass consistent throughout the track, you know? Just a thought, homie.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- Client/src/Pages/Settings/index.jsx (6 hunks)
🧰 Additional context used
🔇 Additional comments (1)
Client/src/Pages/Settings/index.jsx (1)
25-25: Yo, this import's legit!The Dialog component import is on point, sittin' pretty with the other imports. It's like it's always been there, you know what I'm sayin'?
Client/src/Pages/Settings/index.jsx
Outdated
| @@ -38,6 +39,7 @@ const Settings = ({ isAdmin }) => { | |||
| ttl: checkTTL ? (checkTTL / SECONDS_PER_DAY).toString() : 0, | |||
| }); | |||
| const [errors, setErrors] = useState({}); | |||
| const [isOpen, setIsOpen] = useState({delMntrs: false, delSts: false}); | |||
There was a problem hiding this comment.
Yo, this state management's got me sweatin'!
Listen up, homie! This state object's tryna control two dialogs at once, like it's juggling spaghetti or somethin'. It's makin' me nervous, you feel me? Let's break it down:
const [isDeleteMonitorsOpen, setIsDeleteMonitorsOpen] = useState(false);
const [isDeleteStatsOpen, setIsDeleteStatsOpen] = useState(false);This way, we keep it clean and simple, like mom's spaghetti recipe. Each dialog gets its own state, no mess, no fuss. It'll make our code easier to read than rap lyrics, you dig?
Client/src/Pages/Settings/index.jsx
Outdated
| } finally { | ||
| setIsOpen({delMntrs: false, delSts:false}) | ||
| } |
There was a problem hiding this comment.
Yo, this finally block's got more closin' than a rap battle!
Check it, dawg. You're closin' both dialogs like you're shuttin' down the club. But we only opened one, right? Let's keep it tight:
} finally {
setIsOpen(prevState => ({...prevState, delSts: false}));
}This way, we're only closin' the stats dialog, keepin' our state management as clean as a fresh pair of kicks. You feel me?
Client/src/Pages/Settings/index.jsx
Outdated
| } finally { | ||
| setIsOpen({ delMntrs: false, delSts: false }) | ||
| } |
There was a problem hiding this comment.
Yo, we got another overzealous closer here!
Listen up, homie. We're doin' the same thing here as in handleClearStats. We're closin' all the doors when we only opened one. Let's keep it real:
} finally {
setIsOpen(prevState => ({...prevState, delMntrs: false}));
}Now we're only shuttin' down the delete monitors dialog, keepin' our state game strong. It's all about precision, like hittin' those high notes, you know what I'm sayin'?
Client/src/Pages/Settings/index.jsx
Outdated
| <Dialog | ||
| arialabelledby="model-clear-stats" | ||
| ariadescribedby="clear-stats-confirmation" | ||
| open={isOpen.delSts} | ||
| onClose={() => setIsOpen({ delSts: false, delMntrs: false })} | ||
| bdy1="Do you want to clear all stats?" | ||
| yBtnLbl="Yes, clear all stats" | ||
| yBtnOnClick={handleClearStats} | ||
| nBtnLbl="Cancel" | ||
| nBtnOnClick={() => setIsOpen({ delSts: false, delMntrs: false })} | ||
| theme={theme} | ||
| isLoading={isLoading || authIsLoading || checksIsLoading} | ||
| > | ||
| </Dialog> |
There was a problem hiding this comment.
Yo, this Dialog's droppin' like a fresh beat!
This Dialog component's lookin' tight, homie! It's got all the right props, like a rapper with the perfect flow. But peep this:
onClose={() => setIsOpen(prevState => ({...prevState, delSts: false}))}We're closin' both dialogs again when we only need to shut down the stats one. Let's keep our state management as clean as our rhymes, you feel me?
Other than that, this Dialog's ready to drop some confirmation heat on the user! It's gonna make sure they really wanna clear those stats before they do somethin' they can't take back, like spittin' a weak verse on stage.
Client/src/Pages/Settings/index.jsx
Outdated
| onClick={()=>setIsOpen({delMntrs: true, delSts: false})} | ||
| sx={{ mt: theme.spacing(4) }} | ||
| > | ||
| Remove all monitors | ||
| </LoadingButton> | ||
| </Box> | ||
| </Stack> | ||
| <Dialog | ||
| arialabelledby="model-delete-all-monitors" | ||
| ariadescribedby="delete-all-monitors-confirmation" | ||
| open={isOpen.delMntrs} | ||
| onClose={() => setIsOpen({delMntrs:false, delSts: false})} | ||
| bdy1="Do you want to remove all monitors?" | ||
| yBtnLbl="Yes, clear all monitors" | ||
| yBtnOnClick={handleDeleteAllMonitors} | ||
| nBtnLbl="Cancel" | ||
| nBtnOnClick={() => setIsOpen({delMntrs:false, delSts: false})} | ||
| theme={theme} | ||
| isLoading={isLoading || authIsLoading || checksIsLoading} | ||
| > | ||
| </Dialog> |
There was a problem hiding this comment.
Yo, we got a button makeover and a fresh Dialog drop!
Alright, homie, let's break it down:
- That button's got a new style, ditchin' the LoadingButton like last year's kicks. But maybe we wanna keep that loading swag?
<LoadingButton
variant="contained"
color="error"
onClick={() => setIsOpen(prevState => ({...prevState, delMntrs: true}))}
sx={{ mt: theme.spacing(4) }}
>
Remove all monitors
</LoadingButton>- This new Dialog's lookin' fresh, but it's got the same issue as its stats-clearin' cousin:
onClose={() => setIsOpen(prevState => ({...prevState, delMntrs: false}))}We're only dealin' with the monitor deletion dialog here, so let's keep it focused like a tight verse.
Overall, this Dialog's gonna make sure users don't accidentally drop all their monitors like a mic. It's all about that confirmation, you feel me?
@gorkem-bwl sure, I can do that |
|
|
Sorry accidentally closed the PR |
Just would like to reply, will add comments to PR if it has some content , close the PR by mistake |
|
Will work on the review feedbacks and create another PR when done with update . |
|
Attempted to revert the closed PR by mistake |
…-all-stats-and-remove-all-monitors
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (5)
Client/src/Components/Dialog/index.jsx (1)
4-5: Yo, these prop names are making me weak in the knees!Listen up, homie. We gotta make these prop names clearer than mom's spaghetti recipe. How about we switch it up like this:
modelTitle→dialogLabelIdmodelDescription→dialogBodyIdconfirmationBtnLbl→confirmButtonLabelcancelBtnLbl→cancelButtonLabelThis way, we ain't gonna have vomit on our sweater trying to figure out what these props do, ya feel me?
Client/src/Pages/Monitors/Home/actionsMenu.jsx (1)
199-213: Yo, this Dialog component is straight fire, but let's make it even hotter!This new Dialog is looking fresh, dawg! It's got all the bells and whistles, and that
isLoadingstate? chef's kissBut yo, I got a small nitpick that'll make this even more lit:
How about we change
modelTitleandmodelDescriptiontomodalTitleandmodalDescription? It's a modal, not a model, you feel me? Don't want no confusion up in here!- modelTitle="modal-delete-monitor" - modelDescription="delete-monitor-confirmation" + modalTitle="modal-delete-monitor" + modalDescription="delete-monitor-confirmation"Other than that, this Dialog is ready to drop some sick confirmations!
Client/src/Pages/Monitors/Configure/index.jsx (3)
Line range hint
173-174: Yo, this state management is straight fire!Adding that 'isOpen' state? That's some big brain energy right there. It's gonna make controlling that dialog smoother than a fresh jar of Skippy.
But yo, I got a lil' suggestion to make it even more lit:
Consider renaming 'isOpen' to something more specific like 'isDeleteDialogOpen'. It'll make your code read clearer than your mom's recipe for spaghetti, you feel me?
-const [isOpen, setIsOpen] = useState(false); +const [isDeleteDialogOpen, setIsDeleteDialogOpen] = useState(false);
451-465: Yo, this Dialog component is straight fire!Swapping out that old Modal for this new Dialog? That's some next-level UI game right there. It's gonna make our app look fresher than mom's spaghetti on a Sunday morning.
But check it, I got a couple of tweaks to make it even more lit:
- Them prop names are a bit all over the place like spilled spaghetti. Let's keep it consistent, ya feel me?
-modelTitle="modal-delete-monitor" -modelDescription="delete-monitor-confirmation" +modalTitle="modal-delete-monitor" +modalDescription="delete-monitor-confirmation"
- That empty children prop is looking lonelier than an uneaten meatball. If we ain't using it, let's make it a self-closing tag:
- <Dialog - ... - > - </Dialog> + <Dialog + ... + />These changes gonna make your code flow smoother than your mom's secret sauce recipe, no cap.
Line range hint
174-182: Yo, this flow is smoother than a fresh beat, but we can make it even smoother!That handleRemove function? It's not really removing anything anymore, it's just poppin' open that dialog. Let's give it a name that drops the truth like your mom drops spaghetti on the dinner table.
How about we flip the script like this:
-const handleRemove = async (event) => { +const handleOpenDeleteDialog = (event) => { event.preventDefault(); - const action = await dispatch(deleteUptimeMonitor({ authToken, monitor })); - if (action.meta.requestStatus === "fulfilled") { - navigate("/monitors"); - } else { - createToast({ body: "Failed to delete monitor." }); - } + setIsOpen(true); };Then update the button click handler:
-onClick={() => setIsOpen(true)} +onClick={handleOpenDeleteDialog}This way, your code's gonna be clearer than the water you use to boil that spaghetti, you feel me?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- Client/src/Components/Dialog/index.jsx (1 hunks)
- Client/src/Pages/Monitors/Configure/index.jsx (3 hunks)
- Client/src/Pages/Monitors/Home/actionsMenu.jsx (4 hunks)
- Client/src/Pages/Settings/index.jsx (6 hunks)
🧰 Additional context used
🔇 Additional comments (8)
Client/src/Components/Dialog/index.jsx (1)
1-3: Yo, these imports are straight fire!Looks like you've got all the right ingredients for this spaghetti code, dawg. The Material-UI components and PropTypes are on point.
Client/src/Pages/Monitors/Home/actionsMenu.jsx (4)
10-10: Yo, these import changes are fire, dawg!Looks like you've swapped out some of that heavy Material-UI stuff for a custom Dialog. That's some next-level thinking right there!
Also applies to: 19-19
29-29: Yo, this loading state is straight fire!Adding that
isLoadingstate from the Redux store? That's some next-level thinking, homie! It's gonna make our UI smoother than mom's spaghetti.
Line range hint
40-40: Yo, closing that modal is smooth like butter!Shutting down that modal after we delete? That's some slick UX right there, homie! Users gonna be all like "Whoa, that was easy!"
Line range hint
1-230: Yo, this whole component is now smoother than a fresh jar of Skippy!Mad props for keeping the flow tight while upgrading that delete confirmation game. It's like you took mom's spaghetti and turned it into a gourmet pasta dish!
The way you've integrated that new Dialog without messing up the rest of the component? That's some ninja coding right there!
Keep spitting hot fire like this, and we'll have the dopest UI in the game!
Client/src/Pages/Settings/index.jsx (2)
25-25: Yo, this import's fresher than a new beat drop!This Dialog import's comin' in hot, ready to lay down some sweet confirmation flows. It's all good in the hood, homie!
286-290: Yo, this button's got a new trick up its sleeve, and it's smooth like butter!I'm diggin' this new flow, homie! You've got that confirmation dialog poppin' up before we start droppin' monitors like they're hot. It's like havin' a hype man before you start your verse - makin' sure you're ready to go all in. And you kept that loading state too, so we're still in the loop if things start takin' longer than a rap battle.
This is some next-level user experience, my friend. Keep spittin' that hot code!
Client/src/Pages/Monitors/Configure/index.jsx (1)
5-5: Yo, these import changes are fire!Swappin' out that Modal for a custom Dialog component? That's some next-level stuff right there. It's gonna make our app look slicker than mom's spaghetti, no cap.
Also applies to: 27-27
| Dialog.PropTypes = { | ||
| modelTitle: PropTypes.string.isRequired, | ||
| modelDescription: PropTypes.string.isRequired, | ||
| open: PropTypes.bool.isRequired, | ||
| onClose: PropTypes.func.isRequired, | ||
| title: PropTypes.string.isRequired, | ||
| confirmationBtnLbl: PropTypes.string.isRequired, | ||
| confirmationBtnOnClick: PropTypes.func.isRequired, | ||
| cancelBtnLbl: PropTypes.string.isRequired, | ||
| cancelBtnOnClick: PropTypes.func.isRequired, | ||
| theme: PropTypes.object.isRequired, | ||
| isLoading: PropTypes.bool.isRequired, | ||
| description: PropTypes.string | ||
| } |
There was a problem hiding this comment.
Yo, these PropTypes got a typo that's making me lose my spaghetti!
Listen up, dawg. We got a small slip-up here that's gonna make our code trip:
- Dialog.PropTypes = {
+ Dialog.propTypes = {Also, let's make sure our PropTypes are as tight as your rap game:
Dialog.propTypes = {
dialogLabelId: PropTypes.string.isRequired,
dialogBodyId: PropTypes.string.isRequired,
open: PropTypes.bool.isRequired,
onClose: PropTypes.func.isRequired,
title: PropTypes.string.isRequired,
confirmButtonLabel: PropTypes.string.isRequired,
confirmationBtnOnClick: PropTypes.func.isRequired,
cancelButtonLabel: PropTypes.string.isRequired,
cancelBtnOnClick: PropTypes.func.isRequired,
theme: PropTypes.object.isRequired,
isLoading: PropTypes.bool.isRequired,
description: PropTypes.string
}Fix this up, and your code's gonna be cleaner than your favorite rapper's flow, ya feel me?
|
Please review updates in : 21caca1 |
There was a problem hiding this comment.
Looking pretty good! There's a typo in the proptypes and there is a compound state object that should be broken up into its consinstuent states.
Please be quite verbose in your variable names, letters are cheap and it's much easier to understand
deleteMonitorDialogState
vs
delMntrDlgState
Thanks for your hard work!
ajhollid
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
Client/src/Pages/Settings/index.jsx (5)
42-43: Yo, let's make this state name spit some fire!The state management's tight, but the name's weak like mom's spaghetti. How about we drop a more descriptive beat?
const dialogVisibilityInitState = { deleteMonitors: false, deleteStats: false }; const [dialogVisibility, setDialogVisibility] = useState(dialogVisibilityInitState);This way, it's clear as day what we're dealin' with, you feel me?
125-127: This finally block's droppin' the mic right!You're closin' that dialog like it's the end of a rap battle. But let's make it even tighter:
} finally { setDialogVisibility(prevState => ({...prevState, deleteStats: false})); }This way, we're only touchin' the stats dialog, keepin' our state game strong. It's all about precision, like hittin' those high notes, you know what I'm sayin'?
155-157: This finally block's got the same flow, let's make it unique!You're droppin' the same beat as in handleClearStats. Let's give it its own flavor:
} finally { setDialogVisibility(prevState => ({...prevState, deleteMonitors: false})); }Now we're only shuttin' down the delete monitors dialog, keepin' our state game as tight as our rhymes. It's all about that precision, you feel me?
243-256: This Dialog's droppin' like a fresh beat, but let's fine-tune it!Yo, this Dialog's lookin' tight! It's got all the right props, like a rapper with the perfect flow. But peep this:
onClose={() => setDialogVisibility(prevState => ({...prevState, deleteStats: false}))} cancelBtnOnClick={() => setDialogVisibility(prevState => ({...prevState, deleteStats: false}))}We're only dealin' with the stats dialog here, so let's keep it focused like a tight verse. No need to reset both states, you feel me?
Other than that, this Dialog's ready to make sure users don't accidentally clear their stats like droppin' the mic too early!
286-306: This button and Dialog combo's hittin' harder than a bass drop!Yo, check it out! You've given that "Remove all monitors" button a fresh makeover, ditchin' the LoadingButton like last year's kicks. And that new Dialog? It's ready to make sure users don't accidentally drop all their monitors like a mic.
But let's keep our state game as tight as our rhymes:
onClick={() => setDialogVisibility(prevState => ({...prevState, deleteMonitors: true}))} onClose={() => setDialogVisibility(prevState => ({...prevState, deleteMonitors: false}))} cancelBtnOnClick={() => setDialogVisibility(prevState => ({...prevState, deleteMonitors: false}))}This way, we're only messin' with the delete monitors dialog, keepin' our state management cleaner than mom's spaghetti recipe.
Overall, this Dialog's gonna make sure users don't make any moves they can't take back, like spittin' a weak verse on stage. It's all about that confirmation, you feel me?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- Client/src/Components/Dialog/index.jsx (1 hunks)
- Client/src/Pages/Settings/index.jsx (6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Client/src/Components/Dialog/index.jsx
🧰 Additional context used
🔇 Additional comments (2)
Client/src/Pages/Settings/index.jsx (2)
25-25: Yo, this import's fresher than a new beat!This new Dialog import's gonna drop some serious confirmation heat on our users. It's all about that user experience, you feel me?
233-240: This button's got a new style, and it's fire!You've switched up the beat from LoadingButton to Button, and it's smooth like butter. The onClick's now openin' that dialog like it's the hottest club in town. It's all about that user confirmation, you feel me?
| border: 1, | ||
| borderColor: theme.palette.border.light, | ||
| borderRadius: theme.shape.borderRadius, | ||
| boxShadow: 24, |
There was a problem hiding this comment.
In some places we are using theme.shape.boxShadow for this
There was a problem hiding this comment.
We can open up a new PR to handle that, we should go through the whole frontend and look for inconsistencies like that as we migrated to using the theme only a month or two ago
There was a problem hiding this comment.
We could maybe add a TODO just to flag that? Nitpicking, that is not that relevant, but would spare us future work
| border: 1, | ||
| borderColor: theme.palette.border.light, | ||
| borderRadius: theme.shape.borderRadius, | ||
| boxShadow: 24, |
There was a problem hiding this comment.
We can open up a new PR to handle that, we should go through the whole frontend and look for inconsistencies like that as we migrated to using the theme only a month or two ago
ajhollid
deleted the
901-create-confirmation-dialogs-for-clear-all-stats-and-remove-all-monitors
branch
Add confirmation dialogs to delete all monitors and remove all stats upon click the respective buttons