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

Resize code has several issues #50

Closed
mscalora opened this issue Feb 11, 2020 · 6 comments
Closed

Resize code has several issues #50

mscalora opened this issue Feb 11, 2020 · 6 comments

Comments

@mscalora
Copy link
Contributor

Problem Summary

Resize code has several issues

  • Sometimes the table is too wide
  • The table is often more narrow than it needs to be
  • At very small sizes the formatting can be broken

Expected Result (screen shot if possible)

Formatting should always be correct and the table should always be the desired width

Actual Result (screen shot if possible)

See attached output of https://gist.github.com/mscalora/628b05731b7674a1993115752bafd1ee
resize-table.txt

Environment Information

  • OS: MacOS

  • Terminal Emulator: iTerm

Steps to Reproduce

  1. run: curl https://gist.githubusercontent.com/mscalora/628b05731b7674a1993115752bafd1ee/raw/41a1aab155d4235144513eebefe401fe10169456/resize-table.js | node -
  2. examine output

Fixes

I would like to take on fixing these issues and enhancing the table resize functionality with some addition options to better control how resize works:

  • Fix the proportional resize mode issue outline above
  • Add a resize mode that works like CSS flex layout "shrink" mechanism so one can specify which columns should be resized irrespective of content
  • Add a option for desired max output width that will override the terminal width (stdout.columns)
  • Add minimum column width option that can be set on all or specific columns
  • Maybe: Add an option to truncate the whole table if it can't fit in the max output width

Proposed new table options:

    maxTableWidth: "auto" (default) | "none" | number of char
    truncateTable: true (default, if > maxTableWidth) | false | # of char 
    minColWidth: number of char (4 default)
    resizeTable: "proportional" (default) | "shrink" (only used if natural width is > maxTableWidth)

Proposed new column (header) options:

    minWidth: number of char (default is global minColWidth)
    shrink: [optional] used to weight resizing in "shrink" resize mode

Notes:

  • minWidth would be respected by both resize modes

Shrink Mode

  Columns are reduced using the weighting of  shrink / sum(all shrinks)
    Col  |  Shink  | Proportion of resize
     A   |    0    |  none
     A   |    1    |  1/5 or 20%
     A   |    4    |  4/5 or 80%

Discussion

I've actually already started and have most of the work done. I planned on submitting a pull request but wanted to start a discussion and get feedback while I'm still working on it. I understand it may be a a lot of complexity for the scope of this project so I am happy to fork if it is judged to be beyond the desired scope since I need something like this. Also, I can fix some of the issues listed at the top without the new features but it would be a significant amount of extra work to pull that out. I was trying to implement a "legacy" mode but these issues I ran into make that difficult, the broken formatting may only be fixable with some changes like minWidth or using a truncated truncation string.

@tecfu
Copy link
Owner

tecfu commented Feb 11, 2020

Thank you very much for taking the time to thoroughly articulate this issue and your proposed fix. I am delighted that you have chosen to continue to improve this project.

I welcome your improvements and would be happy to merge your pull requests. Based on our interaction thus far, I've decided to add you as a collaborator to the project. This way you can be certain that your changes will land.


Here are my thoughts on your proposal:

[1] I've received some interest in adding a feature to merge columns or rows. I've had no need for this, personally...so haven't tackled it. But I would ask that you consider the inevitability of that additional feature as it would relate to your changes.

[2] Unit tests for said changes are very much appreciated.

[3] I don't have a Changelog in the README, and I've just learned this week about Conventional Commits and will be using that spec from now on. The hope is that through this adoption I'll be able to use conventional changelog to automate a Changelog going forward.

So I would ask that you use this format for your commits, unless you have grounds for objection. I'll add a note in the README to this effect for future PR's.

[4] As opposed to separate options for resizeTable and truncateTable, what do you think about using resizeTable: "truncate" instead?

@mscalora
Copy link
Contributor Author

I think resizeTable and truncateTable are both needed because the different combos would each be useful since I still want to shrink a table as small as the minWidths allow but then truncate to the terminal window size to prevent the ascii mess of wrapped lines. I don't want to loose any resize to prevent the mess with line wrapping. In fact, I want to make truncateTable to default to true since the wrapping mess is so awful and rarely what you want. If the user is redirecting to a file the default tableWidth would be unlimited anyways. would Example of table shrunk to minWidths if the window is too narrow:

Table resized to minWidths:

  ┌────────────────┬───────────────────┬───────────────────────────────────────┐
  │     State      │      Flower       │                 Motto                 │
  ├────────────────┼───────────────────┼───────────────────────────────────────┤
  │  North Dakota  │ Wild prairie rose │ Liberty and union, now and forever, … │
  │ South Carolina │ Yellow jessamine  │ Dum spiro spero Animis opibusque par… │
  │  South Dakota  │ Pasque flower     │ Under God the people rule             │
  │   Tennessee    │ Iris              │ Agriculture and Commerce              │
  │     Texas      │ Bluebonnet        │ Friendship                            │
  │      Utah      │ Sego Lily         │ Industry                              │
  └────────────────┴───────────────────┴───────────────────────────────────────┘

With tableTruncation true

  ┌────────────────┬───────────────────┬──────────────────────────────
  │     State      │      Flower       │                 Motto        
  ├────────────────┼───────────────────┼──────────────────────────────
  │  North Dakota  │ Wild prairie rose │ Liberty and union, now and fo
  │ South Carolina │ Yellow jessamine  │ Dum spiro spero Animis opibus
  │  South Dakota  │ Pasque flower     │ Under God the people rule    
  │   Tennessee    │ Iris              │ Agriculture and Commerce     
  │     Texas      │ Bluebonnet        │ Friendship                   
  │      Utah      │ Sego Lily         │ Industry                     
  └────────────────┴───────────────────┴──────────────────────────────

With tableTruncation false (or no option)

  ┌────────────────┬───────────────────┬──────────────────────────────
─────────┐
  │     State      │      Flower       │                 Motto        
         │
  ├────────────────┼───────────────────┼──────────────────────────────
─────────┤
  │  North Dakota  │ Wild prairie rose │ Liberty and union, now and fo
rever, … │
  │ South Carolina │ Yellow jessamine  │ Dum spiro spero Animis opibus
que par… │
  │  South Dakota  │ Pasque flower     │ Under God the people rule    
         │
  │   Tennessee    │ Iris              │ Agriculture and Commerce     
         │
  │     Texas      │ Bluebonnet        │ Friendship                   
         │
  │      Utah      │ Sego Lily         │ Industry                     
         │
  └────────────────┴───────────────────┴──────────────────────────────
─────────┘

@tecfu
Copy link
Owner

tecfu commented Feb 11, 2020

I am a little concerned that the intended default behavior of tty-table has been misunderstood. Let me explain.

Currently the truncate option defaults to false. Therefore the default behavior of tty-table is to wrap the text within each cell. Even when truncate is set to true, text should never exceed the cell border and the table width should never exceed the size of the viewport at build time. Unless the total column widths have been explicitly specified to do so in the headers options.

Said another way, the only time tableTruncate would ever be needed is when column widths are made explicit and the table width necessarily exceeds the viewport for that reason. That's when the user would see skewed lines as shown in your example above.

I made cell wrapping the default because it ensures all data is displayed, regardless of the user's terminal viewport width. Considering this, if tableTruncation defaults to true it should be meaningless, UNLESS widths are explicitly set - because by design the table should never auto render wider than the viewport.

@tecfu tecfu closed this as completed in 8c4d5f2 Feb 12, 2020
@tecfu
Copy link
Owner

tecfu commented Feb 12, 2020

I pushed fixes for this. You can still submit your pull - I didn't add the tableTruncate feature, but I wanted to deploy a fix for the resize issue.

@mscalora
Copy link
Contributor Author

Yes, good point, the default wouldn't have any affect either way unless the user was getting into more advanced option/headers usage or absurdly narrow terminals.

mscalora pushed a commit to mscalora/tty-table that referenced this issue Feb 12, 2020
@vhscom
Copy link

vhscom commented Jun 10, 2022

The hope is that through this adoption I'll be able to use conventional changelog to automate a Changelog going forward.

Might I recommend changesets given they're using this very library and is very easy to use, with and without conventional commits. 🤗

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

No branches or pull requests

3 participants