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 (textinput): do not resize based on placeholder #442

Merged
merged 1 commit into from Dec 22, 2023

Conversation

seanbanko
Copy link
Contributor

This PR fixes this issue regarding textinput resizing based on the placeholder. The original behavior is documented in the issue and has been modified to the following:

Placeholder = ""
empty-placeholder

Placeholder="123"
short-placeholder

Placeholder = "1234567890"
long-placeholder

Copy link
Member

@maaslalani maaslalani left a comment

Choose a reason for hiding this comment

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

Thank you so much for this PR @seanbanko, we really appreciate it. And thanks for your patience with getting it merged!

@maaslalani maaslalani merged commit 9a0ff2b into charmbracelet:master Dec 22, 2023
9 checks passed
@hopefulTex
Copy link
Contributor

This throws an error in the cases where the Width is set to 0 or is greater than the length of the placeholder.

Placeholder: "7 runes"

Width: 0
runtime error: slice bounds out of range [1:0]
Width: 40
runtime error: slice bounds out of range [:40] with capacity 32

@seanbanko
Copy link
Contributor Author

@hopefulTex thank you for catching this.

I had thought that my test with Width = 5 and Placeholder = "123" would have tested the latter case of Width greater than the length of the placeholder, but I was surprised to learn today that upper bound for a valid slice index in Go is the capacity of the slice, not the length, which I think explains why Width = 5 and Placeholder = "123" was ok but Width = 40 and Placeholder = "7 runes" was not.

I'd be glad to open another PR to fix this unless you'd prefer to; I saw you had one open yourself that may have been more robust than my solution.

@maaslalani
Copy link
Member

Hey @hopefulTex thank you for catching that, I had overlooked before merging. I will revert this PR and fix!

maaslalani added a commit that referenced this pull request Dec 23, 2023
maaslalani added a commit that referenced this pull request Dec 23, 2023
@hopefulTex
Copy link
Contributor

@seanbanko I'll reopen mine since I snuck in an old paste maths fix for a different issue (cause I'm evil like that 😈)

maaslalani pushed a commit to prgres/bubbles that referenced this pull request Feb 28, 2024
Co-authored-by: Sean Banko <seanbanko@Seans-MacBook-Air.local>
maaslalani added a commit to prgres/bubbles that referenced this pull request Feb 28, 2024
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