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(layout): new layout #139

Merged
merged 6 commits into from
Jan 25, 2020
Merged

feat(layout): new layout #139

merged 6 commits into from
Jan 25, 2020

Conversation

imsnif
Copy link
Owner

@imsnif imsnif commented Jan 19, 2020

This is a first draft for solving: #109

It introduces a new layout, middle truncation of long table rows that would otherwise overflow their respective columns (eg. i.am.a.long.hostname would become i am a...stname), and some better (I hope) column spacing inside the tables themselves.

The code is just a draft and the tests will all fail. I'd like to hear some opinions before moving forward with this. If you are reading this and have an opinion - I would very much like to hear it!

Demo:

new-layout

@imsnif imsnif marked this pull request as ready for review January 21, 2020 21:42
@zhangxp1998
Copy link
Collaborator

This layout looks great! Is there anyway to signal the user that domain names are truncated? my.cool...is.long at the first glance look like a normal domain name. Can we color-code it somehow?

@imsnif
Copy link
Owner Author

imsnif commented Jan 21, 2020

This layout looks great! Is there anyway to signal the user that domain names are truncated? my.cool...is.long at the first glance look like a normal domain name. Can we color-code it somehow?

I don't have a good idea how to communicate this with colors, but I definitely see your point... how about if we add square brackets? Something like i.am.a[...]long.hostname?

@imsnif imsnif changed the title WIP: feat(layout): new layout feat(layout): new layout Jan 21, 2020
@zhangxp1998
Copy link
Collaborator

This layout looks great! Is there anyway to signal the user that domain names are truncated? my.cool...is.long at the first glance look like a normal domain name. Can we color-code it somehow?

I don't have a good idea how to communicate this with colors, but I definitely see your point... how about if we add square brackets? Something like i.am.a[...]long.hostname?

Awesome!

@imsnif
Copy link
Owner Author

imsnif commented Jan 21, 2020

Awesome!

Cool - I'll add this (plus a test for it, which I now realize I forgot about) tomorrow

@imsnif
Copy link
Owner Author

imsnif commented Jan 22, 2020

Right - so I made the adjustments. This is ready for a review.

Copy link
Collaborator

@ebroto ebroto left a comment

Choose a reason for hiding this comment

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

IMO this is a major improvement, nice work :)

I've left a minor comment, could be merged without addressing it.


fn truncate_middle(row: &str, max_length: u16) -> String {
if row.len() as u16 > max_length {
let first_slice = &row[0..(max_length as usize / 2) - 2];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not very important as the inputs are literals, but we could enforce that

  • max_length / 2 >= 2
  • row.len() >= max_length / 2 + 2

To avoid invalid indexes

Copy link
Owner Author

Choose a reason for hiding this comment

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

It's a good point... I had this in mind, but couldn't think about what I'd like to happen if that's the case. Do you have any ideas?

Copy link
Collaborator

Choose a reason for hiding this comment

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

hah, good question. This should only happen if the max_length is so small that the [..] itself does not fit and/or the hostname is also too small. Not sure, but maybe just as many dots as it fits, or just a dash?

Copy link
Owner Author

Choose a reason for hiding this comment

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

IMO this can only be a programmer error, since these values are hard-coded. Maybe just an expect so at least at runtime they know what went wrong?

@imsnif
Copy link
Owner Author

imsnif commented Jan 22, 2020

I'll merge this in a few days because I also have to record a new demo gif, and I want to try for one that shows the responsive resizing. That's apparently still a challenge these days. ;P

@imsnif imsnif merged commit 198a1d4 into master Jan 25, 2020
@imsnif imsnif deleted the new-layout branch September 13, 2020 16:28
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