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

Add spacing between the up and down bandwidth values for readability #58

Merged
merged 1 commit into from
Jan 7, 2020

Conversation

Calinou
Copy link
Contributor

@Calinou Calinou commented Jan 2, 2020

This is just an attempt at making bandwidth values more readable. There may be better ways to achieve this, such as color codes.

This kind of tool is all about fast visual grepping, so I figure it's worth trying to find ways to improve readability 🙂

Preview

Bandwidth up/down

@imsnif
Copy link
Owner

imsnif commented Jan 2, 2020

Heh, interesting :)

I'll play around with this some more in the next few days to see how it behaves in all the different window size layouts, but so far I like it.

One note for now: I'd also do this for the column title.

All the tests are obviously failing, but if we decide to move forward with them - I can fix them quickly. (and maybe also take the opportunity to write a doc about how to do that with cargo insta).

Copy link
Owner

@imsnif imsnif left a comment

Choose a reason for hiding this comment

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

Hey - so first: my apologies for the late review and for splitting it into two. I promise this is the last one and we can merge after we resolve a couple of issues.

I left one comment in the code, and the second one is more functional:
With this change, when I played around with the terminal window size, the Rate column gets a little cut off for me in the smallest supported width.
What I came up with that worked is this:

-const FIRST_COLUMN_WIDTHS: [u16; 4] = [20, 30, 40, 50];
+const FIRST_COLUMN_WIDTHS: [u16; 4] = [10, 30, 40, 50];
 const SECOND_COLUMN_WIDTHS: [u16; 1] = [20];
-const THIRD_COLUMN_WIDTHS: [u16; 4] = [10, 20, 20, 20];
+const THIRD_COLUMN_WIDTHS: [u16; 4] = [20, 20, 20, 20];

It can be further optimized if you feel creative. :)

About the tests: this repo uses insta for snapshot tests. In case you don't know it, in order to fix the tests, you can run them, have the new snapshots created and then review each one by running cargo insta (I should definitely document this :) ).

If you'd be willing to also fix the tests, that would be great as I could never quite get github to accept my pushes to other people's forks. I'm probably doing something wrong, but this will help me out. :)

Otherwise, as I mentioned, I like this change a lot. So even if you don't have the time to work on this, I'll likely get to it at some point.

src/display/ui.rs Outdated Show resolved Hide resolved
@Calinou Calinou force-pushed the improve-bandwidth-display branch 2 times, most recently from e1d2cdc to 4ed4a92 Compare January 4, 2020 17:17
@imsnif
Copy link
Owner

imsnif commented Jan 5, 2020

Looks like rustfmt is complaining again :) Could you run it once more please?

@imsnif imsnif merged commit 288a6b8 into imsnif:master Jan 7, 2020
@imsnif
Copy link
Owner

imsnif commented Jan 7, 2020

Thankyou very much for this!

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.

2 participants