Skip to content
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

feat: mutiple incidents #4986

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Conversation

joebnb
Copy link

@joebnb joebnb commented Aug 2, 2024

⚠️⚠️⚠️ Since we do not accept all types of pull requests and do not want to waste your time. Please be sure that you have read pull request rules:
https://github.com/louislam/uptime-kuma/blob/master/CONTRIBUTING.md#can-i-create-a-pull-request-for-uptime-kuma

Tick the checkbox if you understand [x]:

  • I have read and understand the pull request rules.

Description

Feat: #4953

As previously mentioned, we want uptime-kuma to support displaying multiple incidents simultaneously.

Changes required:

  • Remove the action of unpinning all incidents on the current status page when creating a new incident.
  • When unpinning, include the incident ID to unpin the specific incident.
  • Change the incident status in the statusPage to return an array.
  • Additionally, make the necessary UI changes on status page.

Type of change

Please delete any options that are not relevant.

  • User interface (UI)
  • New feature (non-breaking change which adds functionality)

Checklist

  • My code follows the style guidelines of this project
  • I ran ESLint and other linters for modified files
  • I have performed a self-review of my own code and tested it
  • I have commented my code, particularly in hard-to-understand areas (including JSDoc for methods)
  • My changes generates no new warnings
  • My code needed automated testing. I have added them (this is optional task)

Screenshots (if any)

image

image

image

image

incognito window
image

deletion
image

Please do not use any external image service. Instead, just paste in or drag and drop the image here, and it will be uploaded automatically.

Copy link
Collaborator

@CommanderStorm CommanderStorm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work 👍🏻

I have left some minor change-reuquests, but those should be fairly straitforeward.

incident = incident.toPublicJSON();
}
// Incident List
const incidentList = await StatusPage.getIncidentList(statusPage.id);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In cases such as these two lines, please use Promise.all() to not await twice (=a bit faster)

We know that this is an issue accross the app and that performance could be improved this way ^^

@@ -4,6 +4,7 @@ dist
dist-ssr
*.local
.idea
.history/
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could not find good docs what the folder does.
What does this ignore do?

Suggested change
.history/

@@ -69,14 +65,15 @@ module.exports.statusPageSocketHandler = (socket) => {
}
});

socket.on("unpinIncident", async (slug, callback) => {
socket.on("unpinIncident", async (slug, incident, callback) => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please change this to be just the incidentId, as incident.id is the only part from this which is used

Comment on lines +1084 to +1088
if (this.incidentList[index].content != null) {
return DOMPurify.sanitize(marked(this.incidentList[index].content));
} else {
return "";
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets simplify our live a bit

Suggested change
if (this.incidentList[index].content != null) {
return DOMPurify.sanitize(marked(this.incidentList[index].content));
} else {
return "";
}
const markedContent = marked(this.incidentList[index].content ?? "");
return DOMPurify.sanitize(markedContent);

{{ $t("Last Updated") }}: {{ $root.datetime(incident.lastUpdatedDate) }} ({{ dateFromNow(incident.lastUpdatedDate) }})
</span>
</div>
<template v-for="(incidentItem, index) in incidentList" :key="incidentItem.id">
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that the incidents should be refactored into their own component. This is getting somewhat messy currently (as can be seen on editIncidentMode(..), incidentClass and others existing ^^)

return publicIncidentList;

} catch (error) {
return [];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you add a warning log here? (seems scarry to just ignore the error, I know that you did it to match the above)

<button id="dropdownMenuButton1" class="btn btn-secondary dropdown-toggle" type="button" data-bs-toggle="dropdown" aria-expanded="false">
{{ $t("Style") }}: {{ $t(incidentItem.style) }}
</button>
<ul class="dropdown-menu" aria-labelledby="dropdownMenuButton1">
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The dropdown menu looks sort of buggy.
Could you remove the margin?

image

</ul>
</div>

<button v-if="!editIncidentMode(index) && incidentItem.id" class="btn btn-light me-2" @click="unpinIncident(index)">
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For extra points (not required):

The current UI for these buttons is not mobile friendly and needs a bit of tinkering

image

@CommanderStorm CommanderStorm added pr:please address review comments this PR needs a bit more work to be mergable area:status-page Everything related to the status page labels Aug 8, 2024
@CommanderStorm CommanderStorm changed the title Feat:mutiple incident feat: mutiple incidents Oct 4, 2024
@rettler-pupil
Copy link

Hi @joebnb

What is the status on PR?

@CommanderStorm CommanderStorm marked this pull request as draft October 7, 2024 13:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:status-page Everything related to the status page needs:resolve-merge-conflict pr:please address review comments this PR needs a bit more work to be mergable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants