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

Custom metrics when viewing device #1804

Merged
merged 3 commits into from
Jan 23, 2025
Merged

Conversation

lawik
Copy link
Contributor

@lawik lawik commented Jan 21, 2025

Adds custom metrics for display:
Screenshot_20250121_091250

@lawik lawik requested review from joshk and nshoes January 21, 2025 08:27
@lawik
Copy link
Contributor Author

lawik commented Jan 21, 2025

This is the more controversial part of #1796 and we don't have to merge it. I think it demos well. I think it is more useful than just showing our preferred ones. But not a hill I'll die on, made the PR as much to save the generic code since we may want it on the Health view :D

@joshk
Copy link
Collaborator

joshk commented Jan 21, 2025

I'd be happy to merge this in if we are to combine it with an opt-in in the Product settings to show all or 'extended' metrics. (off by default)

Long term, I'd like to add a settings icon (cog) to the bottom right which gives you a way to customize what metrics you see on the device page.

@lawik
Copy link
Contributor Author

lawik commented Jan 21, 2025

Yeah, I am on board with some kind of product-wide setting to control what shows up. I'll noodle on something.

@nshoes
Copy link
Contributor

nshoes commented Jan 22, 2025

I'd be happy to merge this in if we are to combine it with an opt-in in the Product settings to show all or 'extended' metrics.

+1 for me as well!

@nshoes nshoes changed the title Issue 1796 custom metrics on display Custom metrics when viewing device Jan 22, 2025
@lawik
Copy link
Contributor Author

lawik commented Jan 22, 2025

Want to do the setting thing. But more fiddly to get the database stuff set up. So for now removing the custom ones from the details view but including it in health. So now if you choose to look at health you get an overview of the latest data before all the graphs.

Screenshot 2025-01-22 at 23 26 55

My Mac doesn't report health properly. But I essentially copy+pasted from details and then adjusted a little bit.

We can do the settings bit in a later PR. Let me know what you think :)

@joshk
Copy link
Collaborator

joshk commented Jan 22, 2025

I love this! I had intended to add this to the health tab, thank you!

@joshk
Copy link
Collaborator

joshk commented Jan 22, 2025

Ah, sorry, didn't see that it hadn't been pushed to the PR yet 😅

I know its a little none standard, I'm wondering if we remove the border and title and just have the stats.

We can also move the 'last updated' next to the period select buttons?

@lawik lawik force-pushed the issue-1796-custom-metrics-on-display branch from cb748b0 to 56e6918 Compare January 23, 2025 07:50
@lawik
Copy link
Contributor Author

lawik commented Jan 23, 2025

Did I forget to commit and just pushed air? Maybe a little..

@lawik lawik force-pushed the issue-1796-custom-metrics-on-display branch from 56e6918 to ac7d446 Compare January 23, 2025 10:09
@joshk joshk force-pushed the issue-1796-custom-metrics-on-display branch from ac7d446 to 8c0c661 Compare January 23, 2025 20:02
@joshk
Copy link
Collaborator

joshk commented Jan 23, 2025

I've made a small adjustment to your PR:

Screenshot 2025-01-24 at 9 22 33 AM

I removed the headers and link, and moved the 'last reported at' to the metrics header. I tried this without any border around the stats section, but i felt it wasn't keeping with the design.

I like this a bit better, and would be happy to merge it in.

@joshk joshk merged commit 70e4f50 into main Jan 23, 2025
2 checks passed
@joshk joshk deleted the issue-1796-custom-metrics-on-display branch January 23, 2025 20:32
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