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

Graphs for Dashboard #69

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

Graphs for Dashboard #69

wants to merge 6 commits into from

Conversation

Its4Nik
Copy link

@Its4Nik Its4Nik commented May 21, 2024

Added:

  • Toggleable Graphs via Mouse-click
  • Some Margins to make it fit better

Changed:

  • Moved timespan below the Graph

Mainly (thank youuuu) done by: @Devsider, a good friend of mine.

image
Just imagine the timespan dropdown below the chart.
And it's better looking than that screenshot, but it's really late here so I won't bother to make new screenshots today.

@Its4Nik Its4Nik changed the title Graphs for Dasshboard Graphs for Dashboard May 21, 2024
@Its4Nik
Copy link
Author

Its4Nik commented May 22, 2024

Also implemented the logic that if a Monitor is a Ping Monitor, it doesnt have the graph property

Copy link
Contributor

@moonrailgun moonrailgun left a comment

Choose a reason for hiding this comment

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

Yeah, thanks your change but its break too much things.

and i think click is not a good idea, because not all people wanna show it.

the best way for it is add a page options to switch it, if people want it.(without click, because no one will know it at first).

(here):
image

the other thing is trpc.monitor.data not a public request. because we dont wanna everyone can visit all data, because maybe its a private data. we should have a new route if we wanna show to public and check is this monitor under current page

Copy link
Contributor

Choose a reason for hiding this comment

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

emmmm why delete docker compose file?

Copy link
Author

Choose a reason for hiding this comment

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

i deleted it locally because i had another one and there were conflicts, my bad

Copy link
Contributor

Choose a reason for hiding this comment

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

what is those 4 file for?

Copy link
Author

Choose a reason for hiding this comment

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

idk where they came from, i think when i built the project those just popped up

pnpm-lock.yaml Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

i think you should not submit this file

@@ -157,7 +157,8 @@ export const MonitorDataChart: React.FC<{ monitorId: string }> = React.memo(

return (
<div>
<div className="mb-4 text-right">
<Area {...config} />
Copy link
Contributor

Choose a reason for hiding this comment

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

i think you need prettier.

@@ -157,7 +157,8 @@ export const MonitorDataChart: React.FC<{ monitorId: string }> = React.memo(

return (
<div>
<div className="mb-4 text-right">
<Area {...config} />
Copy link
Contributor

Choose a reason for hiding this comment

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

i think you need prettier.

and why move its order?

// onClick={onClick}
onClick={() => {
setShowGraph(!showGraph);
}}
Copy link
Contributor

Choose a reason for hiding this comment

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

this not a good idea because you break normal list logic.

here:
image


// Add the MonitorDataChart component to be shown
export const MonitorDataChartComponent = () => {
return <MonitorDataChart />;
Copy link
Contributor

Choose a reason for hiding this comment

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

we dont need dashboard anymore

@Its4Nik
Copy link
Author

Its4Nik commented May 22, 2024

Okay so except those issues, from the design just taking the already existing chart is fine?

@moonrailgun
Copy link
Contributor

Okay so except those issues, from the design just taking the already existing chart is fine?

yeah, current chart is enough. did you have other idea?

@Its4Nik
Copy link
Author

Its4Nik commented May 24, 2024

Okay so except those issues, from the design just taking the already existing chart is fine?

yeah, current chart is enough. did you have other idea?

well my first Intuition was chartjs or sonething else but that would just be annoying to implement imo. If there is already a chart in the same design why not use it

@moonrailgun
Copy link
Contributor

👍 free feel to re request again after you fixed all problem

@SalimQS
Copy link

SalimQS commented Jun 18, 2024

Okay so except those issues, from the design just taking the already existing chart is fine?

please, can u continue this, i really think this is a good idea

@Its4Nik
Copy link
Author

Its4Nik commented Jun 18, 2024

Okay so except those issues, from the design just taking the already existing chart is fine?

please, can u continue this, i really think this is a good idea

I am working on it, but i have internet problems at home so i dont have much time. But I am going to work on it

@SalimQS
Copy link

SalimQS commented Jun 19, 2024

Okay so except those issues, from the design just taking the already existing chart is fine?

please, can u continue this, i really think this is a good idea

I am working on it, but i have internet problems at home so i dont have much time. But I am going to work on it

i hope the best for you, good luck :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants