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

Calendar graph #4419

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

Conversation

akhileshsooji028
Copy link

@akhileshsooji028 akhileshsooji028 commented Jan 24, 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

Fixes #1984

Type of change

Please delete any options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • User interface (UI)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (a fix or feature that would cause existing functionality to not work as expected)
  • Other
  • This change requires a documentation update

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

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.

From the changes you made I am mostly not getting the why and think that this PR was created somewhat accidentally (?):

  • Why did you choose said libraries, was this not possible with chart.js?
  • You seem to not have used tippy.js. Is this intentional?
  • You commented out some code. Was this for testing purposes or is there a reason?
  • Please do fill out the description we provide. I have added the template.

@CommanderStorm CommanderStorm marked this pull request as draft January 24, 2024 10:24
@akhileshsooji028
Copy link
Author

akhileshsooji028 commented Jan 24, 2024

  1. In the Issue npm package calendar-graph was mentioned so the similar dedicated vue package has been used that is vue3-calendar-heatmap
  2. Tippy.js is used to show the tooltip on hovering on the chart.

@akhileshsooji028 akhileshsooji028 marked this pull request as ready for review January 24, 2024 10:53
@akhileshsooji028 akhileshsooji028 marked this pull request as draft January 24, 2024 10:55
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.

I have had the time to look over this PR and have made a few inline comments about some defects and other stuff.

In general:

  • to be mergable, the design needs to fit into the current design. Currently, you are using blue accents. Please try to keep the design close to our
  • Sorry if we were unclear about this part of the UI having to be configurable if significant changes like this one are made
  • Have you tested that this works as intended? See my concearns about the ´shortBeatList´.
  • While I like the way this looks, I am confused what a counts is. Please display normal metrics like daily uptime (in %) instead.

<div>{{ timeSinceFirstBeat }} ago</div>
<div v-if="$root.styleElapsedTime === 'with-line'" class="connecting-line"></div>
<div>{{ timeSinceLastBeat }}</div>
<CalendarHeatmap :style="{ fill: '#fff' }" class="heat-map" :values="values" :end-date="endDate" no-data-text="Down" tooltip-unit="counts" />
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here you removed the existing beat list.
Please make what is displayed configurable in the sidebars appreeance settings

src/components/PublicGroupList.vue Outdated Show resolved Hide resolved
src/components/HeartbeatBar.vue Outdated Show resolved Hide resolved
src/components/HeartbeatBar.vue Outdated Show resolved Hide resolved
src/components/HeartbeatBar.vue Outdated Show resolved Hide resolved
package.json Outdated
@@ -142,7 +142,9 @@
"tar": "~6.1.11",
"tcp-ping": "~0.1.1",
"thirty-two": "~1.0.2",
"tippy.js": "^6.3.7",
Copy link
Collaborator

Choose a reason for hiding this comment

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

A bit more context is needed on this one:
We already have popperjs installed.
Is there a reason why you chose this library?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, the Vue-Calendar-Heatmap has suggested to use it as mentioned in the documentation.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we use popperjs instead for these popups?

Choose a reason for hiding this comment

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

@CommanderStorm Tippy.js seems to be a peer dependency to the calendar heat-map package. As per their documentation this is a required dev dependency. I also tried disabling tooltip and removing the tippy.js dependency, that throw an error ERROR: Could not resolve "tippy.js". For now I have moved them to the dev dependency.

@CommanderStorm CommanderStorm added the question Further information is requested label Jan 24, 2024
@chakflying

This comment was marked as resolved.

@CommanderStorm

This comment was marked as resolved.

@CommanderStorm CommanderStorm mentioned this pull request Jan 26, 2024
1 task
@CommanderStorm
Copy link
Collaborator

In my opinion, we should drop this feature as the Current Graph stand with the standard of Uptime Kuma UI and one of the features which gives clean look to Uptime Kuma.

As long as this is configurable and is well integrated, such an alternate display mode is fine by me.
This is the design decision we chose for the last few features, which were design related and might be considered controversial.

@CommanderStorm
Copy link
Collaborator

by "alternate display mode" I mean that in the sidebar appearance settings a setting should be added that allows switching between the bigger calendar graph and the shorter beats graph.

@akhileshsooji028 akhileshsooji028 marked this pull request as ready for review February 6, 2024 08:05
@CommanderStorm CommanderStorm requested review from chakflying and removed request for CommanderStorm February 6, 2024 22:46
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.

This has improved, but not to the level we require.
These points from my previous review are not resolved.

@@ -80,20 +80,32 @@ router.get("/api/status-page/heartbeat/:slug", cache("1 minutes"), async (reques
]);

for (let monitorID of monitorIDList) {
// Calculating the percentage of uptime per day with the status count to show full year data
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is better (removing XY clounts), but still not what I meant when I said

in the sidebar appearance settings a setting should be added that allows switching between the bigger calendar graph and the shorter beats graph.

You switched all monitors to the calendar.
Please make this configurable either on

  • a per monitor (see the show link feature and the badge generator) or
  • per status page level.

ORDER BY COUNT(*) DESC
LIMIT 1
) as status
FROM heartbeat as main
Copy link
Collaborator

Choose a reason for hiding this comment

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

@chakflying You have a better grasp at this (this is why the review-request).
Is this query correct? Should this not use stat_daily instead?

Copy link
Collaborator

@chakflying chakflying Feb 7, 2024

Choose a reason for hiding this comment

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

Yes, in the near future, the heartbeats table will only keep data for the last 48 hours. The getDataArray function in UptimeCalculator should be used instead. The raw counts data can retrieved that way, and the precentage calculated in the front-end.

Since this was only implemented recently, with the only component utilizing it (PingChart) still in PR state (#4264), any feedback on the implementation is welcome.

Copy link
Author

Choose a reason for hiding this comment

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

@chakflying correct me if i'm wrong, In near future ur gonna save only 48 hrs monitoring data right. so if I use getDataArray function, Will I get the last 365 days monitoring data?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The getDataArray function does not use the heartbeats table, and instead uses the various stats_ tables. You can take a look at the function implementation as it is in master. We have no time limit when using the 'day' interval.

     getDataArray(num, type = "day") {
        if (type === "hour" && num > 24 * 30) {
            throw new Error("The maximum number of hours is 720");
        }
        if (type === "minute" && num > 24 * 60) {
            throw new Error("The maximum number of minutes is 1440");
        }
     ...

@CommanderStorm CommanderStorm marked this pull request as draft February 6, 2024 23:24
@CommanderStorm CommanderStorm added the area:status-page Everything related to the status page label Feb 12, 2024
@CommanderStorm CommanderStorm added the pr:please address review comments this PR needs a bit more work to be mergable label May 19, 2024
@github-actions github-actions bot added the pr:please resolve merge conflict A merge-conflict needs to be addressed before reviewing makes sense again label Jun 23, 2024
@github-actions github-actions bot removed the pr:please resolve merge conflict A merge-conflict needs to be addressed before reviewing makes sense again label Jul 19, 2024
@github-actions github-actions bot added the pr:please resolve merge conflict A merge-conflict needs to be addressed before reviewing makes sense again label Aug 19, 2024
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 pr:please resolve merge conflict A merge-conflict needs to be addressed before reviewing makes sense again question Further information is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Calendar Graph
4 participants