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

fix: port breakLines to wrap-ansi #1106

Merged
merged 4 commits into from Apr 26, 2022
Merged

Conversation

starpit
Copy link
Contributor

@starpit starpit commented Apr 24, 2022

This PR addresses incorrect line wrapping with terminal-link. It does so by employing the wrap-ansi npm.

Fixes #1014
Fixes #304

To the reviewers: I wasn't sure whether we should pass the hard option to wrap-ansi:

By default the wrap is soft, meaning long words may extend past the column width. Setting this to true will make it hard wrap at the column width.

If inquirer needs hard wrapping (in which case I assume wrap-ansi may split long words at arbitrary points in order to avoid overflow), we can easily add that to this PR.

@starpit starpit force-pushed the wrap-ansi branch 2 times, most recently from 8640d55 to d5a8d29 Compare April 24, 2022 15:47
This PR addresses incorrect line wrapping with `terminal-link`. It does so by employing the `wrap-ansi` npm.

Fixes SBoudrias#1014
Fixes SBoudrias#304
@SBoudrias
Copy link
Owner

Thanks for sending a PR; this looks very promising.

We'll need the hard option as terminal line returns are unfortunately messing up cursor position detection; which is why we implemented the breakLines feature in the first place.

@codecov
Copy link

codecov bot commented Apr 25, 2022

Codecov Report

Merging #1106 (4644d33) into master (aad7190) will decrease coverage by 0.13%.
The diff coverage is 100.00%.

❗ Current head 4644d33 differs from pull request most recent head d74a9f4. Consider uploading reports for the commit d74a9f4 to get more accurate results

@@            Coverage Diff             @@
##           master    #1106      +/-   ##
==========================================
- Coverage   88.30%   88.16%   -0.14%     
==========================================
  Files           4        4              
  Lines         171      169       -2     
  Branches       23       22       -1     
==========================================
- Hits          151      149       -2     
  Misses          1        1              
  Partials       19       19              
Impacted Files Coverage Δ
packages/core/lib/utils.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aad7190...d74a9f4. Read the comment docs.

@SBoudrias
Copy link
Owner

Would you mind also addressing the new screen manager located at /packages/core/lib/screen-manager.js?

- move wrapAnsi dependence out of the top-level
@starpit
Copy link
Contributor Author

starpit commented Apr 25, 2022

Thanks for the great feedback. I have made the requested changes. Let me know if you need me to squash the commits. Unsure of your preference, I left them unsquashed for now.

Copy link
Owner

@SBoudrias SBoudrias left a comment

Choose a reason for hiding this comment

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

LGTM!

One extra thing if you have some time and don't mind, it'd be great to include a demo with terminal-link. I think it'll be hard to write an automated test; so at least a demo would help validate and caught regression when doing manual checks pre-releases.

@starpit
Copy link
Contributor Author

starpit commented Apr 25, 2022

Sure, i'll add an example. I'll have to use terminal@2, since terminal@latest is ESM-only.

@starpit
Copy link
Contributor Author

starpit commented Apr 25, 2022

Expected line breaks (with this PR)

terminal-link correct line breaks

Buggy line breaks (prior to this PR)

terminal-link bad line breaks

@SBoudrias SBoudrias merged commit b8c2aa0 into SBoudrias:master Apr 26, 2022
@SBoudrias
Copy link
Owner

Awesome! Thanks a ton for following through with everything 🎉

@starpit starpit deleted the wrap-ansi branch April 26, 2022 19:17
starpit added a commit to starpit/Inquirer.js that referenced this pull request Apr 27, 2022
[This PR](SBoudrias#1106) updated `breakLines` to use `wrap-ansi`. However, in doing so, we broke the paginator. It assumes that the return value will be of the same length as the original array, but that broken lines will be subarrays.
SBoudrias pushed a commit that referenced this pull request Apr 28, 2022
[This PR](#1106) updated `breakLines` to use `wrap-ansi`. However, in doing so, we broke the paginator. It assumes that the return value will be of the same length as the original array, but that broken lines will be subarrays.
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.

Using terminal-link causes question text to get broken into two lines Use wrap-ansi to force wrap lines
2 participants