Implement "Silence List" Feature as a Modal #539
Conversation
bai1024
commented
Aug 20, 2024
bai1024
commented
- Change the current list interface to a modal design.
- Within the modal, Provide table about Silence list
- Introduce filter feature within the modal interface.
bai1024
force-pushed
the
implement-silence-list-modal
branch
from
2c5946e to
0404de6
Compare
promgen/static/js/promgen.vue.js
Outdated
| hideModal() { | ||
| const modal = $('#silenceTableModal'); | ||
| if (modal.length) { | ||
| globalStore.setMessages([]); | ||
| this.form.labels = []; | ||
| modal.modal('hide'); | ||
| } | ||
| }, | ||
| showModal() { | ||
| const modal = $('#silenceTableModal'); | ||
| if (modal.length) { | ||
| modal.on('hidden.bs.modal', () => { | ||
| silenceTableStore.hideModal(); | ||
| }); | ||
| modal.modal('show'); | ||
| } | ||
| }, |
There was a problem hiding this comment.
This is a question asked out of ignorance, but is $ in this case a jQuery object or something built into Vue?
I know we're eventually planning on removing jquery. Typically I thought we were using native javascript object queries for things like this.
There was a problem hiding this comment.
In this case, $ is indeed jQuery, not something built into Vue. We're using it here because our Modal is still using Bootstrap 3, which relies on jQuery for DOM manipulation and event triggering.
| activeSilences() { | ||
| return this.$root.activeSilences || []; | ||
| }, |
There was a problem hiding this comment.
Since we can get a list from activeSilences() it seems like we could do a quick filter on that, to provide a list of suggestions for the matcher label field, and then do something similar for the matcher value based on which label is selected 🤔
There was a problem hiding this comment.
ah, I see. I will look into it.
bai1024
force-pushed
the
implement-silence-list-modal
branch
2 times, most recently
from
815a953 to
f317ba2
Compare
There was a problem hiding this comment.
So now instead of being able to write a custom matcher, we offer the user a selection of existing labels and values. That makes it easier since the user does not even need to type, but removes the possibility to write a regex.
Anyway, for the sake of simplicity I am fine with it. If we want, in the future, we can improve that area to support regex. No problem at all 👍
-
I have made one code suggestion related to the behavior of the filtering, which acts like a logical
OR: it returns all silences that match with any of the matchers.
We want that filtering mechanism to work in the same way as Alertmanager, which is like a logicalAND.
The rest of the code is fine by me. -
Regarding Git stuff, please put in separate commit all the changes related to the renaming of
silence-modalintosilence-create-modal.
Good job :)
promgen/static/js/promgen.vue.js
Outdated
| } | ||
|
|
||
| return this.activeSilences.filter(silence => { | ||
| return this.state.labels.some(filterLabel => { |
There was a problem hiding this comment.
| return this.state.labels.some(filterLabel => { | |
| return this.state.labels.every(filterLabel => { |
If we have multiple filters, we want all of them to match. This is the same behavior that Alertmanager has.
There was a problem hiding this comment.
Thank you.
And for this one
Regarding Git stuff, please put in separate commit all the changes related to the renaming of silence-modal into silence-create-modal.
Is it better to make a different PR for that?
There was a problem hiding this comment.
Is it better to make a different PR for that?
It is not necessary. Even if they are unrelated changes, it is totally fine to start PRs with refactor commits to better accommodate your actual changes.
So, having one commit for the rename, and another commit for all the changes related to the new silence list modal is fine.
There was a problem hiding this comment.
No problem. Updated.
There was a problem hiding this comment.
I have tested it in my local environment and works well 👍
Nice job :)
bai1024
force-pushed
the
implement-silence-list-modal
branch
from
1cc310c to
236aa46
Compare
bai1024
force-pushed
the
implement-silence-list-modal
branch
from
b53a2b8 to
3a5a31a
Compare
