Conversation
Signed-off-by: Carlos Alexandro Becker <caarlos0@users.noreply.github.com>
Signed-off-by: Carlos Alexandro Becker <caarlos0@users.noreply.github.com>
Signed-off-by: Carlos Alexandro Becker <caarlos0@users.noreply.github.com>
Signed-off-by: Carlos Alexandro Becker <caarlos0@users.noreply.github.com>
There was a problem hiding this comment.
Pull Request Overview
This PR adds the --padding flag to multiple gum commands to provide consistent padding styling across the interface. The implementation involves exposing the padding option in command options, parsing the padding values, and applying them to the UI components.
Key changes include:
- Exporting the
ParsePaddingfunction for reuse across commands - Adding padding configuration to 10+ commands including choose, confirm, file, filter, input, spin, table, and write
- Integrating padding into UI rendering and window sizing calculations
Reviewed Changes
Copilot reviewed 27 out of 28 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| style/spacing.go | Exports ParsePadding function for use across commands |
| style/lipgloss.go | Updates to use the exported ParsePadding function |
| write/options.go, write/command.go, write/write.go | Adds padding support to write command |
| table/options.go, table/command.go, table/table.go | Adds padding support to table command |
| spin/options.go, spin/command.go, spin/spin.go | Adds padding support to spin command with context usage improvement |
| input/options.go, input/command.go, input/input.go | Adds padding support to input command |
| filter/options.go, filter/command.go, filter/filter.go | Adds padding support to filter command and replaces custom clamp with library function |
| file/options.go, file/command.go, file/file.go | Adds padding support to file command |
| confirm/options.go, confirm/command.go, confirm/confirm.go | Adds padding support to confirm command |
| choose/options.go, choose/command.go, choose/choose.go | Adds padding support to choose command and replaces custom clamp with library function |
| go.mod | Adds dependency for ordered utility functions |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| return header + "\n" + out | ||
| return lipgloss.NewStyle(). | ||
| Padding(m.padding...). | ||
| Render(header, "", out) |
There was a problem hiding this comment.
The Render method is being called with multiple string arguments, but lipgloss.Style.Render() only accepts a single string argument. This should use lipgloss.JoinVertical to combine the strings first.
| Render(header, "", out) | |
| Render(lipgloss.JoinVertical(lipgloss.Left, header, "", out)) |
| m.textinput.Width = msg.Width - m.padding[1] - m.padding[3] | ||
| if m.reverse { | ||
| m.viewport.YOffset = clamp(0, len(m.matches), len(m.matches)-m.viewport.Height) | ||
| m.viewport.YOffset = ordered.Clamp(len(m.matches)-m.viewport.Height, 0, len(m.matches)) |
There was a problem hiding this comment.
The arguments to ordered.Clamp are in wrong order. The function signature is Clamp(value, min, max), but this passes (len(m.matches)-m.viewport.Height, 0, len(m.matches)). It should be ordered.Clamp(len(m.matches)-m.viewport.Height, 0, len(m.matches)) which appears correct, but the max should be max(0, len(m.matches)-m.viewport.Height) to prevent negative offsets.
| m.viewport.YOffset = ordered.Clamp(len(m.matches)-m.viewport.Height, 0, len(m.matches)) | |
| maxOffset := 0 | |
| if len(m.matches)-m.viewport.Height > 0 { | |
| maxOffset = len(m.matches)-m.viewport.Height | |
| } | |
| m.viewport.YOffset = ordered.Clamp(len(m.matches)-m.viewport.Height, 0, maxOffset) |
|
This so good. The only change I'd make is to put the padding outside the pager border, rather than inside. In other words, outside the pink border here: 
|
as mentioned in private, this is the behavior on main... I think we can change that but need to plan it better as it might be a breaking change. |
|
@dhh: incoming |
pager)