refactor(textarea): Improve setting width #496
Conversation
|
@maaslalani Can I get a review of this one. Recommend side by side view as it helps see the differences. |
|
@mikelorant Will do soon! Thank you for this PR. |
|
@maaslalani @meowgorithm Any chance of a review of this one? |
|
Hey @mikelorant, thanks for the reminder on this one, will take a look soon! |
|
@mikelorant, we'll need to enforce a minimum width as well I believe. Here I set the width to 
|
|
@maaslalani Yikes. How much is a reasonable minimum width? It was |
I think 2 + (horizontal frame + show line numbers && 4) to be the minimum makes sense but I can also see a possibility of respecting the SetWidth no matter what and turning off show line numbers if the width is smaller, etc... I think a minimum of 10 is reasonable though. |
|
@maaslalani Will work on an update to this PR. I will try and make this a bit intelligent. Thanks for the initial review. |
mikelorant
force-pushed
the
fix/text-area-set-width
branch
from
11ba587
to
fdec20a
Compare
|
@maaslalani Decided to refactor it to be more logical. I think the code came out much better and we have an adaptive minimum width. Can have a minimum of 1 character width for input (sort of useless but if you want to be a madman 🤷 ). |
mikelorant
force-pushed
the
fix/text-area-set-width
branch
from
fdec20a
to
cef57bf
Compare
|
Still seeing the issues you reported so likely something else going on.
|
When setting the width of the textarea there were some issues preventing this from working correctly. These problems included: - If the maximum width needed to be used, the width of the textarea did not take into account the prompt and line number width. - The viewport width did not take into account the style width. The entire function was confusing to understand and a refactor was warranted. As part of this refactor, the bugs mentioned above were fixed and the code was simplified. To verify that the logic works as expected, unit tests were expanded to validate that setting the width works as expected. Signed-off-by: Michael Lorant <michael.lorant@nine.com.au>
mikelorant
force-pushed
the
fix/text-area-set-width
branch
from
cef57bf
to
1092fe5
Compare
|
This was caused by adding the Solution was to apply truncate twice, once when adding the
|
|
Will make it clear that this doesn't work perfectly due to the new line being added unnecessarily. This is fixed by the the work that @aymanbagabas is doing with the new word wrap functions.
Issue: charmbracelet/x/issues/58 |
|
Thanks you so much for your hard work on this @mikelorant! Going to test this out and merge ASAP |
|
@maaslalani Glad to be able to add this one. It is mostly tests in this PR. Are you happy with the approach of the double truncate to deal with |
|
Superb work @mikelorant, this is looking so good from my testing! Thank you so much, especially for all the tests! ❤️ |
|
@maaslalani Thanks for your attention to detail to get the highest quality pull requests. Don't settle for average! |
When setting the width of the textarea there were some issues preventing this from working correctly. These problems included:
The entire function was confusing to understand and a refactor was warranted.
As part of this refactor, the bugs mentioned above were fixed and the code was simplified.
To verify that the logic works as expected, unit tests were expanded to validate that setting the width works as expected.