-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Add SLO cloning functionality #149935
Add SLO cloning functionality #149935
Conversation
Pinging @elastic/actionable-observability (Team: Actionable Observability) |
@@ -92,7 +92,7 @@ export const sloList: FindSLOResponse = { | |||
}, | |||
{ | |||
...baseSlo, | |||
id: 'c0f8d669-9177-4706-9098-f397a88173a6', | |||
id: 'c0f8d669-9277-4706-9098-f397a88173a6', |
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.
I noticed React was complaining about duplicate ID's when running tests, this fixes that
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.
Ah yes good catch I forgot to change the id for this one 🙈
16ae86e
to
8b50209
Compare
💚 Build Succeeded
Metrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: |
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.
LGTM!
Just a remark regarding finally block that sets success to false every time. Otherwise my other nit was addressed before I finished the review :p
} finally { | ||
setSuccess(false); | ||
setLoading(false); |
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.
I think finally
is called regardless of the success or failure of the try. So in this case, we always set success
to false.
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.
Yeah, I was experimenting a bit with this pattern... will most likely revert
const handleCloning = (cloning: boolean) => { | ||
setIsCloning(cloning); | ||
}; | ||
|
||
const handleDeleting = (deleting: boolean) => { | ||
setIsDeleting(deleting); |
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.
nit: passing the boolean state feels strange when reading the code, especially from the modal component: e.g. onDeleting(true)
or onDeleting(false)
Maybe we can keep onDeleting()
without boolean that sets the state to true, and the handleClonedOrDeleted
can reset both state (deleting and cloning) to false?
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.
Indeed, this was exactly what I thought as well. Tbh, this is going to change if and when we move to React Query: it will have less prop plumbing because we can just listen for the api interaction finishing inside the list component.
Resolves #149917
📝 Summary
This PR adds the functionality to clone an existing SLO. It also adds some more tests for the SLO List page.
Looks like this:
Screen.Recording.2023-01-31.at.14.13.38.mov
✅ Checklist