feat(manager): move and update metrics cards in the server view #2241
Conversation
daniellacosse
commented
Oct 14, 2024
•
daniellacosse
commented
daniellacosse
changed the title
daniellacosse
changed the title
daniellacosse
force-pushed
the
daniellacosse/tunneltime/move_metrics
branch
from
55108a4 to
ee8c388
Compare
There was a problem hiding this comment.
As discussed offline, Per-ASN breakdown was a significant gap during the Iran crisis. It's a confirmed need and and we put a lot of effort to implement it. It's more of a question of how, not whether we want to surface that.
I would like to see that surfaced somehow in this PR, to inform the UX design. We've discussed a simple table, which seems to work well.
Sure thing, working on it! |
Here's an initial PR for a data table component: #2273 |
daniellacosse
changed the title
server_manager/www/testing/models.ts
Outdated
| @@ -168,6 +168,34 @@ export class FakeServer implements server.Server { | |||
| getDataUsage() { | |||
| return Promise.resolve(new Map<server.AccessKeyId, number>()); | |||
| } | |||
| getServerMetrics() { | |||
There was a problem hiding this comment.
Perhaps throw an error instead. The tests should define what to return instead.
There was a problem hiding this comment.
Just to be clear - by extending this mock and overriding the method? Or via injection?
I see an example below!
There was a problem hiding this comment.
On that note, I'd like to see some tests for the 2 scenarios (supported and unsupported), probably in app.spec.ts?
There was a problem hiding this comment.
"supported" here meaning with the experimental/server/metrics endpoint present or with getServerMetrics method implemented?
There was a problem hiding this comment.
Yes basically what happens when the API doesn't return anything (it's presumably missing).
| { | ||
| icon: 'timer', | ||
| name: this.localize('server-metrics-user-hours'), | ||
| units: this.localize('server-metrics-user-hours-unit'), |
There was a problem hiding this comment.
This is not really how we should be formatting units. Use the standard libraries instead: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/NumberFormat/NumberFormat#unit_2
There was a problem hiding this comment.
I think we need a follow up PR that fixes this and breaks out the units that are baked into the messages (e.g. / last 30 days) as mentioned above. It just needs to be rethought, as I assumed relying on the localizer was enough. I'll make a ticket for it, fair?
There was a problem hiding this comment.
Actually, looking through the code, all I really need to do here is format the time unit with the formatter I think?
Co-authored-by: Vinicius Fortuna <[email protected]>
Co-authored-by: Vinicius Fortuna <[email protected]>
server_manager/www/testing/models.ts
Outdated
| @@ -168,6 +168,34 @@ export class FakeServer implements server.Server { | |||
| getDataUsage() { | |||
| return Promise.resolve(new Map<server.AccessKeyId, number>()); | |||
| } | |||
| getServerMetrics() { | |||
There was a problem hiding this comment.
On that note, I'd like to see some tests for the 2 scenarios (supported and unsupported), probably in app.spec.ts?
|
After testing with the new canary server, this is basically working, except the fact that the data flow in our UI is bad and not triggering a render as it should. (to get it to show up in the UI in the above screenshot I had to edit the code to trigger a live reload LMAO) We should be passing data through HTML attributes not via JS properties... Anyway, I'll return after the holidays to fix that, format the hourly units properly, and add a unit test. |
