refactor(textarea): Improve setting width

charmbracelet / bubbles

TUI components for Bubble Tea 🫧

MIT License

5.35k stars 252 forks source link

refactor(textarea): Improve setting width #496

Closed mikelorant closed 5 months ago

mikelorant commented 6 months ago

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.

mikelorant commented 5 months ago

@maaslalani Can I get a review of this one. Recommend side by side view as it helps see the differences.

maaslalani commented 5 months ago

@mikelorant Will do soon! Thank you for this PR.

mikelorant commented 5 months ago

@maaslalani @meowgorithm Any chance of a review of this one?

maaslalani commented 5 months ago

Hey @mikelorant, thanks for the reminder on this one, will take a look soon!

maaslalani commented 5 months ago

@mikelorant, we'll need to enforce a minimum width as well I believe.

Here I set the width to 6 which breaks since it's smaller than the border + line numbers + prompt.

mikelorant commented 5 months ago

@maaslalani Yikes. How much is a reasonable minimum width? It was 2 before and that was silly. Or should we calculate it?

maaslalani commented 5 months ago

@maaslalani Yikes. How much is a reasonable minimum width? It was 2 before and that was silly. Or should we calculate it?

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.

mikelorant commented 5 months ago

@maaslalani Will work on an update to this PR. I will try and make this a bit intelligent.

Thanks for the initial review.

mikelorant commented 5 months ago

@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 commented 5 months ago

Still seeing the issues you reported so likely something else going on.

textarea

mikelorant commented 5 months ago

This was caused by adding the ... for the placeholder when truncated.

Solution was to apply truncate twice, once when adding the ... and again to make sure we don't exceed the minimum width.

textarea-fixed

mikelorant commented 5 months ago

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.

new-line-bug

Issue: charmbracelet/x/issues/58 Fixed-by: charmbracelet/x/pull/59

maaslalani commented 5 months ago

Thanks you so much for your hard work on this @mikelorant!

Going to test this out and merge ASAP

mikelorant commented 5 months ago

@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 ... not fitting?

maaslalani commented 5 months ago

Superb work @mikelorant, this is looking so good from my testing! Thank you so much, especially for all the tests! ❤️

mikelorant commented 5 months ago

@maaslalani Thanks for your attention to detail to get the highest quality pull requests. Don't settle for average!