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

feat(manager): use server metrics row #2354

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

Conversation

daniellacosse
Copy link
Contributor

@daniellacosse daniellacosse commented Jan 31, 2025

Screen.Recording.2025-01-31.at.3.29.41.PM.mov

@github-actions github-actions bot added size/XL and removed size/L labels Jan 31, 2025
@daniellacosse daniellacosse marked this pull request as ready for review January 31, 2025 20:24
@daniellacosse daniellacosse requested review from fortuna and a team as code owners January 31, 2025 20:24
@daniellacosse daniellacosse requested a review from sbruens January 31, 2025 20:28
@fortuna fortuna removed their request for review January 31, 2025 22:36
serverView.totalInboundBytes = totalInboundBytes;
const NUMBER_OF_ASES_TO_SHOW = 4;
serverView.bandwidthUsageTotal = this.formatGigabyteValueAndUnit(
bandwidthUsageTotal / BYTES_IN_GIGABYTES
Copy link
Collaborator

Choose a reason for hiding this comment

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

You should pick the unit based on the value. Otherwise you will end up with zeros

.reverse()
.map(server => ({
title: server.asOrg,
subtitle: `${server.asn}AS`,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Swap. AS must come before the number

return '';
}

return new Intl.NumberFormat(this.appRoot.language, {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use our existing formatting library to properly pick the units

.top(NUMBER_OF_ASES_TO_SHOW)
.reverse()
.map(server => ({
title: server.asOrg,
Copy link
Collaborator

Choose a reason for hiding this comment

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

You don't always have asOrg. What's the plan when it's absent?
I usually use ${asOrg} (AS${asn}) with fallback to AS${asn}

@@ -1101,11 +1172,80 @@ export class App {
}
}

private formatGigabyteValueAndUnit(data: number) {
// This happens during app startup before we set the language
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 this is because we're not setting the default property properly. If you set the default value, you shouldn't need this https://polymer-library.polymer-project.org/3.0/docs/devguide/properties#configure-values

countryCode.length !== 2 ||
!/^[A-Z]{2}$/.test(countryCode)
) {
return 'Invalid country code';
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs localization.

private countryCodeToEmoji(countryCode: string) {
if (
!countryCode ||
countryCode.length !== 2 ||
Copy link
Contributor

Choose a reason for hiding this comment

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

This length check seems redundant given the regular expression also checks the length.

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

Successfully merging this pull request may close these issues.

3 participants