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): wrong placeholder/completion rendering #560

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

luevano
Copy link
Contributor

@luevano luevano commented Jul 18, 2024

Fixes:

  • Completion using Placeholder style (only the cursor was using the Completion style)
  • Extra padding after completion.
  • Wrong completion char under cursor when overflowing (using index of viewable viewport instead of the completion)

TODO/discuss:

  • What should the Width apply to? both the input text + cursor? only the text? the whole thing with the prompt? Because as of right now, the actual width of the view is Width + cursor + prompt

Some examples, using contrasting colors for easier identification. Showing a half box below just to see the width, and appended a "Ñ" to see the end of the input view.

Before fix:
Width set to 10, showing the placeholder color:
image
Extra padding after completion (what should be appended without completion), and the color under the cursor is what should be displayed for the whole completion:
image
Wrong completion character showing under cursor when overflowing (should show a 'k'):
image

After fix:
Correct completion style applied:
image
Correct completion char used under cursor when overflowing:
image

@meowgorithm
Copy link
Member

Thanks, @luevano. Could you provide some code we can use to see the before and after?

@luevano
Copy link
Contributor Author

luevano commented Jul 22, 2024

Thanks, @luevano. Could you provide some code we can use to see the before and after?

Hi @meowgorithm, no problem, made it as short as possible (ctrl+o: toggle size; ctrl+c: quit):

package main

import (
	"github.com/charmbracelet/bubbles/textinput"
	tea "github.com/charmbracelet/bubbletea"
	"github.com/charmbracelet/lipgloss"
)

type Model struct {
	input textinput.Model
	long  bool

	size,
	sizeShort int
	end string
}

func newModel() *Model {
	size, sizeShort := 64, 10
	input := textinput.New()
	input.Placeholder = "Long placeholder..."
	input.ShowSuggestions = true
	input.Width = size
	input.PlaceholderStyle = lipgloss.NewStyle().Foreground(lipgloss.Color("#008080"))
	input.CompletionStyle = lipgloss.NewStyle().Foreground(lipgloss.Color("#EB5E28"))

	input.SetSuggestions([]string{
		"hello",
		"world",
		"some long suggestion",
		"another very long suggestion...",
	})

	return &Model{
		input:     input,
		long:      true,
		size:      size,
		sizeShort: sizeShort,
		end:       lipgloss.NewStyle().Foreground(lipgloss.Color("#FF0000")).Render("Ñ"),
	}
}

func (m *Model) Init() tea.Cmd {
	m.input.CursorEnd()
	return tea.Sequence(
		m.input.Focus(),
		textinput.Blink,
	)
}

func (m *Model) Update(msg tea.Msg) (tea.Model, tea.Cmd) {
	switch msg := msg.(type) {
	case tea.KeyMsg:
		switch msg.Type {
		case tea.KeyCtrlC:
			return m, tea.Quit
		case tea.KeyCtrlO:
			m.long = !m.long
			if m.long {
				m.input.Width = m.size
			} else {
				m.input.Width = m.sizeShort
			}
		}
	}

	input, cmd := m.input.Update(msg)
	m.input = input
	return m, cmd
}

func (m *Model) View() string {
	return m.input.View() + m.end
}

func main() {
	if _, err := tea.NewProgram(newModel()).Run(); err != nil {
		panic(err)
	}
}

@caarlos0 caarlos0 added the bug Something isn't working label Aug 13, 2024
@bashbunni bashbunni added this to the v0.20.0 milestone Aug 19, 2024
@bashbunni bashbunni self-assigned this Aug 19, 2024
@caarlos0 caarlos0 modified the milestones: v0.20.0, v0.21.0 Sep 6, 2024
@bashbunni
Copy link
Member

bashbunni commented Oct 11, 2024

Hey @luevano sorry for the delay on this. The changes look great. I think width should include everything (even cursor + prompt) as that will be the most predictable behaviour. That said, I'll ask the team to see if they have any thoughts on this too.

@bashbunni
Copy link
Member

Update, team agreed that the width should contain all components including the text field, cursor, and prompt

@luevano
Copy link
Contributor Author

luevano commented Oct 13, 2024

@bashbunni thanks for the feedback, I agree with that.
I did, however, try to fix the whole thing to fit within the given width, fut failed miserably lol I did try to do that back when I was working on this fix, will give it another go and see what I can do.

@bashbunni
Copy link
Member

@luevano no problem! I can see about fixing the width :) thanks again for your work on this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants