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

Code refactoring #58

Merged
merged 9 commits into from
Sep 6, 2021
Merged

Conversation

Caesarovich
Copy link
Collaborator

@Caesarovich Caesarovich commented Aug 30, 2021

This PR has for objective to provide a little code refactoring in order to make the library more easily maintainable/scalable/readable.
Effectively the main function of the library was around 150 lines long and it renders it really hard to read/understand.

So I decided to divide it a little, now the code is separated into two 'main functions' :

  • The main function from before without the content handling part.
  • And the makeContentText function wich now handles the whole content part.

This new function takes in 4 arguments:

  • text: wich is the box's content text provided by the user
  • padding: the optional padding desired
  • columns: the maximum column count the content should take
  • align: the optional text alignment

This allows for more flexibility regarding the width of the content. It'll allow an easy implementation for issue #51. (PR coming soon). Also the way this function is done/used fixes #53 and all kind of similar bugs where the box would exceed the terminal's size. See images below:

Image comparison of old/new version

This pretty much sums up the purpose of this PR. It's kind of a preparation for other PRs I'm preparing


Fixes #53

index.js Outdated Show resolved Hide resolved
@sindresorhus
Copy link
Owner

Remove outdated tests that aren't relevant anymore

Why are the tests not relevant anymore?

@sindresorhus
Copy link
Owner

Also the way this function is done/used fixes #53 and all kind of similar bugs where the box would exceed the terminal's size.

Would be nice to have tests for this.

Reduce two lines into one

Co-authored-by: Sindre Sorhus <sindresorhus@gmail.com>
@Caesarovich
Copy link
Collaborator Author

Remove outdated tests that aren't relevant anymore

Why are the tests not relevant anymore?

Those tests were expecting results that do not accord with the fact that the boxes shouldn't overflow the terminal.

I found that by investigating those and try running them in a separate script. I observed that the test produced a box exceeding the terminal's size and accounting it as Ok, while accounting the new version (The one that doesn't overflow anymore) as Ko.
It's due to the fact that the new way of doing the box I implemented works very differently than before in some ways, like cutting the text into newlines to not overflow, etc...

@Caesarovich
Copy link
Collaborator Author

Also the way this function is done/used fixes #53 and all kind of similar bugs where the box would exceed the terminal's size.

Would be nice to have tests for this.

This is a very good idea, I'll get to it.

@Caesarovich
Copy link
Collaborator Author

Changes done, waiting for your review 👍

@sindresorhus
Copy link
Owner

Those tests were expecting results that do not accord with the fact that the boxes shouldn't overflow the terminal.

Wouldn't it be better to adjust the tests instead of removing them?

@Caesarovich
Copy link
Collaborator Author

Changes done, waiting for your review

Those tests were expecting results that do not accord with the fact that the boxes shouldn't overflow the terminal.

Wouldn't it be better to adjust the tests instead of removing them?

I can do it. Tho the it is now equivalent as combining the 'align option' tests with the new 'overflow' test. If anything, one one these two would already Ko before.

I'll push it in the next minutes.

@Caesarovich
Copy link
Collaborator Author

Readapted the tests. I pushed the changes. 👌

test.js Outdated Show resolved Hide resolved
test.js Outdated Show resolved Hide resolved
@sindresorhus sindresorhus merged commit c6f9801 into sindresorhus:main Sep 6, 2021
@sindresorhus
Copy link
Owner

Looks good now. Thanks :)

@Caesarovich Caesarovich deleted the code-refactoring branch September 6, 2021 14:29
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.

Text wrapping doesn't account for margin
2 participants