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

Fixed the bugs in displaying the grid with fit_into_width #14

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

Conversation

mtsoltan
Copy link

@mtsoltan mtsoltan commented Oct 9, 2021

These changes were initiated to fix the behavior of term-grid in uutils's ls, which uses term-grid 0.1.7 but can't upgrade to 0.2.0 due to the existence of these bugs.

Ideally, I'd like to upgrade to 0.2.0, offloading a lot of padding/alignment code to term-grid instead.

Things I've taken care of:

  • Fixed loop boundaries.
  • Allowed grid to try to utilize the maximum width it can, optimizing for line-count.

Things I've not taken care of, yet:

  • I didn't create a test to make sure this behavior does not regress.
  • I didn't fix the other bug where terminal color-codes ruin the width calculation.

@untitaker
Copy link

untitaker commented Jul 24, 2022

@ogham is it possible for those changes to be merged? 0.2.0 feels fundamentally broken because of the presence of those bugs. In fact it seems like term-grid gives up on producing a grid if the content has to produce more than one row in any way.

I can confirm that 0.1.7 works fine for me, and so does 0.2.0 after applying this patch.

@tertsdiepraam
Copy link

@mtsoltan We've finally decided to make a fork of this library for uutils: https://github.com/uutils/uutils-term-grid. Feel free to reopen this PR there!

@mtsoltan
Copy link
Author

@tertsdiepraam
On it once I wake up (in 9 hours)~

@mtsoltan
Copy link
Author

mtsoltan commented Dec 3, 2022

Forgot about this, just did it now! Sorry~

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