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

ability to specify minColumnWidth for Help.wrap #1430

Merged
merged 2 commits into from Jan 11, 2021
Merged

ability to specify minColumnWidth for Help.wrap #1430

merged 2 commits into from Jan 11, 2021

Conversation

cravler
Copy link
Contributor

@cravler cravler commented Jan 6, 2021

Pull Request

@shadowspawn
Copy link
Collaborator

shadowspawn commented Jan 8, 2021

Do you have a situation when you want to be able to change minColumnWidth? Or suggesting it because it is currently a hard-coded value in the code?

For historical interest, this was actually a parameter in the original PR! I removed it rather than try and add documentation for the parameter which we do not vary in the Commander code, as I struggled to differentiate it from the "width" parameter. The current variable name is the best I came up with at the time.

@cravler
Copy link
Contributor Author

cravler commented Jan 9, 2021

yes, i want set it to minColumnWidth=0, so it would be totally disabled

i played with different terminal sizes and app looks better when it disabled

minColumnWidth=0:
image

minColumnWidth=40
image

@cravler
Copy link
Contributor Author

cravler commented Jan 9, 2021

maybe better to make like this?

class Help {
    formatHelp(cmd, helper) {
        //...

        function wrap(str, width, indent) {
            // Do not wrap if not enough room for a wrapped column of text (as could end up with a word per line).
            const columnWidth = width - indent;
            const minColumnWidth = 40;
            if (columnWidth < minColumnWidth) return str;
            return return helper.wrap(str, width, indent);
        }

        function formatItem(term, description) {
          if (description) {
            const fullText = `${term.padEnd(termWidth + itemSeparatorWidth)}${description}`;
            return wrap(fullText, helpWidth - itemIndentWidth, termWidth + itemSeparatorWidth);
          }
          return term;
        };

        // ...
    }
}

@shadowspawn
Copy link
Collaborator

The reason for skipping the wrap at small sizes is that in the worst case can get a word per line, and unlike the fallback unwrapped output, this does not reflow as the terminal window is made wider as it is hard wrapped. e.g.

  daemon              Daemon
                      to
                      listen
                      to
                      multiple
                      sockets
                      connections
  init [name] [path]  Created
                      a new
                      application

But being able to configure the cut-off is a reasonable request. Still thinking about that.

@shadowspawn
Copy link
Collaborator

shadowspawn commented Jan 10, 2021

I do not think this will be be used much, but it isn't a front-facing routine and this hopefully makes wrap more usable for some custom overrides. Ok.

If you are still keen: add the TypeScript, and I'll add some comments when I merge it if it passes review.

@shadowspawn shadowspawn changed the base branch from release/7.x to develop January 10, 2021 04:47
Copy link
Collaborator

@shadowspawn shadowspawn left a comment

Choose a reason for hiding this comment

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

LGTM. I'll add an explanation of the new parameter after merging.

Copy link
Collaborator

@abetomo abetomo left a comment

Choose a reason for hiding this comment

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

👍

@shadowspawn shadowspawn self-assigned this Jan 11, 2021
@shadowspawn shadowspawn merged commit fcc8988 into tj:develop Jan 11, 2021
@shadowspawn shadowspawn added the pending release Merged into a branch for a future release, but not released yet label Jan 11, 2021
@shadowspawn shadowspawn added this to the v7.0.0 milestone Jan 11, 2021
@shadowspawn shadowspawn removed the pending release Merged into a branch for a future release, but not released yet label Jan 16, 2021
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.

None yet

3 participants