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

[Feature] Added averages and date ranges to the panel dashboard #1669

Open
wants to merge 26 commits into
base: main
Choose a base branch
from

Conversation

svenvg93
Copy link
Contributor

@svenvg93 svenvg93 commented Aug 14, 2024

📃 Description

This PR introduces new functionalities and updates the data display on the dashboard.
The public dashboard is out of scope for this pr

Please double check the working of the time picker and env value before releasing to make sure it works as intended.

🪵 Changelog

➕ Added

✏️ Changed

  • Let the charts no longer start at 0. This gives a better view of changes in speeds on high rates.
  • Fill the charts with 10% opacity to not have just a single line.
  • When there are more than 5 data points in a time range, the dots are hidden. This prevents the chart from appearing cluttered and ensures that the average line remains visible, especially when the connection is stable.

📷 Screenshots

357473416-b3584d00-40d0-4c04-992c-fc2669352e8b

@svenvg93 svenvg93 changed the title Add new fucntions to the Dashboard Add new Functions to the Dashboard Aug 14, 2024
@alexjustesen alexjustesen added the 🎉 feature New feature or request label Aug 15, 2024
@alexjustesen
Copy link
Owner

This is awesome man, great work! Couple of quick comments:

  1. Should be provide so many date range options when a custom one is available?
  2. What should a sensible default be for the charts, it's 24h currently but I think switching to a week makes more sense with averages.
  3. We should probably use the DateTimePicker to allow people to select time frames and not just full dates should they want to get granular.

On the topic of charts starting at 0 vs. within range of values. This has been a hot topic in the past and it comes down to preference, let's add an environment variable to control this instead.

@svenvg93
Copy link
Contributor Author

svenvg93 commented Aug 15, 2024

Thanks for the feedback! See below my answers, please let me know your thoughts.

  1. Should be provide so many date range options when a custom one is available?

No we don't, In one of the requests there was 3h,6h etc.. We could just only show the custom one, or only keep 24h, 1 week, 1 month next to the costom one.

  1. What should a sensible default be for the charts, it's 24h currently but I think switching to a week makes more sense with averages.

I think it depends how often you test. When you test once a day, having a week makes sense. When you test every hour having 1 day makes sense. Thats one of the reasons I made it a env, so people can set it to there liking.

  1. We should probably use the DateTimePicker to allow people to select time frames and not just full dates should they want to get granular.

I experimented with DateTimePicker but couldn't get it to work reliably, especially with setting the hours. I'll give it another try.

On the topic of charts starting at 0 vs. within range of values. This has been a hot topic in the past and it comes down to preference, let's add an environment variable to control this instead.

Good idea, I will add it. Just wondering if we should do the same for the filling of the graph and the dots then.
Did it cross my mind when making it. But also didn't wanted to crazy with those options.

@alexjustesen
Copy link
Owner

I experimented with DateTimePicker but couldn't get it to work reliably, especially with setting the hours. I'll give it another try.

Ok, let's skip that for now then and we can circle back around to it in a patch.

Good idea, I will add it. Just wondering if we should do the same for the filling of the graph and the dots then.
Did it cross my mind when making it. But also didn't wanted to crazy with those options.

I think I avoided fill back when the graphs were combined, it's just UI so I think we can leave it out of configurable options for now. When you add the config for starting at zero can you default to 0 so there is no change from current?

@svenvg93
Copy link
Contributor Author

svenvg93 commented Aug 15, 2024

Done! Added the env for the begin at 0, it will default to true so there is indeed no change.
'chart_begin_at_zero' => env('CHART_BEGIN_AT_ZERO', true),`

I will remove some of the pre-defined time ranges to keep it short

@alexjustesen
Copy link
Owner

alexjustesen commented Aug 15, 2024

One other quick note, can you change chart_time_range to chart_default_date_range this feels more descriptive of what the variable does. Using date here feels right as we're defining whole days in this option.

We'll also need to look into how to port this over to the public dashboard.

@svenvg93
Copy link
Contributor Author

svenvg93 commented Aug 15, 2024

Done as well :)

Just to be sure the fill for the graph does not need to be a environment?

We'll also need to look into how to port this over to the public dashboard.

Yess, im sure how the public dashboard is build. Some guidiness on that would be appreciated :)

@alexjustesen
Copy link
Owner

Correct, let's skip the fill option.

For the public dashboard we can do that work in a separate PR.

@svenvg93
Copy link
Contributor Author

svenvg93 commented Aug 15, 2024

I played with the failed test in the charts. But im not sure it will look good in the charts. As it need to be added to all of them as well. Maybe having a Pie chart would make more sense. Or just dont have iti in the charts.
image

or this (i have to figure out the dots )
image

@alexjustesen
Copy link
Owner

Let's extract the failed changes to a separate PR, helps me do reviews faster if I'm only testing 1 or a couple things. That being said I think I like the gap in charting more. We could introduce a new chart that's a "failure rate" progress bar chart to show success/failure rates.

I'm happy to work on that one separately.

@svenvg93
Copy link
Contributor Author

Sounds like a better plan. Lets keep it with what we have now and do the otherthings later :).

@alexjustesen
Copy link
Owner

I'll be able to test the PR tonight

@svenvg93
Copy link
Contributor Author

svenvg93 commented Aug 26, 2024

I have been trying to figure out why this happens, but no result so far. At the same time Im thinking user might not change the chart_default_date_range that often. Would assume it will only be set when setting up the container.

@svenvg93
Copy link
Contributor Author

svenvg93 commented Sep 2, 2024

In the latest update, I switched to using the Filament date picker to ensure a consistent layout. Additionally, I separated the date picker into its own component, making it easier to reuse on other pages.

Was able to get the DataTimePicker to work :)
Also add a maxTicksLimit of 25 to make sure that the x Axis is not to crowed when selecting a long range

image

@alexjustesen
Copy link
Owner

I'm going to give this a functionality review later tonight, thanks for all the work!

@svenvg93
Copy link
Contributor Author

Thanks man! One other idea I had was to remove to the predefined 24h, 1 week, 1 month and just only have the date/time picker.

Copy link
Owner

@alexjustesen alexjustesen left a comment

Choose a reason for hiding this comment

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

We're also going to have to add these dashboard changes to the public dashboard in this release. I'll do some research on how to wrap the form and chart updates into that view.

app/Forms/Components/DateFilterForm.php Show resolved Hide resolved
@svenvg93
Copy link
Contributor Author

@alexjustesen Not sure why it keeps saying that there are requested changes, already resolved the finding you noticed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment