lipgloss
lipgloss copied to clipboard
charmbracelet
feat: New Border Title API
Adds new Border Title API (requested in the issue #87):
-
func (s Style) BorderTitle(title string) Style -
func (s Style) BorderTitleAlignment(alignment Position) Style -
func (s Style) BorderTitleBackground(color TerminalColor) Style -
func (s Style) BorderTitleForeground(color TerminalColor) Style -
func (s Style) GetBorderTitle() string -
func (s Style) GetBorderTitleAlignment() Position -
func (s Style) GetBorderTitleBackground() TerminalColor -
func (s Style) GetBorderTitleForeground() TerminalColor
Updated example:
Once the new API is agreed upon and stable, I will also update the documentation
@maaslalani @meowgorithm Is there anything else needed to make this PR a part the next release?
Hey, @eugener! We just need a little time to give this a proper review. Thanks for your patience with this one; we'll definitely get to it.
Another question I had, was would we want to support Bold / Italic / Underline / Padding / Margin in the border title?
I almost wonder if the border title should take its own style or take only a string which we expect the user to style before passing in?
I.e:
borderTitle := lipgloss.NewStyle().Bold(true).Underline(true).Render("Hello, Borders")
lipgloss.NewStyle().BorderTitle(borderTitle).Border(lipgloss.DoubleBorder())
Another question I had, was would we want to support Bold / Italic / Underline / Padding / Margin in the border title?
I almost wonder if the border title should take its own style or take only a string which we expect the user to style before passing in?
I.e:
borderTitle := lipgloss.NewStyle().Bold(true).Underline(true).Render("Hello, Borders") lipgloss.NewStyle().BorderTitle(borderTitle).Border(lipgloss.DoubleBorder())
The idea of styled text is great and also makes the Title API surface smaller. Can you suggest what changes are required to implement this, assuming we will remove Fg and Bg colors API? It seems that the title can now be multiline, if it has its own border for example, which has to be taken into consideration
I think there's two ways to do it, either we can get passed the styled text or take the BorderTitleStyle as an argument and then apply the BorderTitle as an Inline Style. I.e. remove the padding and margin.
The current API doesn't prevent people from passing in multi-line text (if I'm not mistaken), so I think it is reasonable to expect the user to pass in a non-multiline string / inline style. But, @meowgorithm may have more thoughts on this.
Personally, I think it would be reasonable to remove the BorderTitleForeground and BorderTitleBackground from the API and expect people to pass in a pre-styled one-line string which includes horizontal padding / fg / bg, etc...
What do you think @meowgorithm @eugener? I'm not too opinionated on this one. Although, the way I have suggested does give the user more ways to break their application's layout, so maybe it's not worth the flexibility.
That being said, I am also fully onboard with the current PR as it is. I think it is currently in a shippable state.
We will need to offer at least the basic ANSI styling options for this (bold, underline, strikethrough, reverse, and so on) so it probably makes sense to pass a style. It would also give people more control over the spacing around the title.
And we do have mechanisms to keep help prevent people from breaking stuff (see Enforcing Rules in the README as well as the various unset methods).
So if we go down this route you'd either pass a styled string:
borderEverything := lipgloss.NewStyle().
Bold(true).
Underline(true).
Render("Hello, Borders")
style := lipgloss.NewStyle().
BorderTitle(borderEverything).
Border(lipgloss.DoubleBorder())
Or you'd separate concerns a bit more and set the style and the string separately:
borderTitleStyle := lipgloss.NewStyle().
Bold(true).
Underline(true)
style := lipgloss.NewStyle().
BorderTitleStyle(borderStyle).
BorderTitle("Hello, Borders").
Border(lipgloss.DoubleBorder())
And, in this scenario, I'd keep the very good BorderTitleAlignment.
@maaslalani @meowgorithm Thank you both for very insightful suggestions!
Based on your suggestions, I redesigned the API by passing the Style instead of a text, which eliminated the need for all API methods except get/set BorderTitle. Currently, the title text is set using style.SetString, which seems to be fine to me comparing to introducing a separate method for setting border title text on Style, but I'm open for discussion on this topic.
New API allows for full title styling including horizontal alignment. Example and docs are up-to-date also.
I think using the SetString and the HorizontalAlign API is quite clever, you basically get the best of both worlds with the first (singular entry point) and second approach (flexibility and control of the Style). Awesome work!
This is great @eugener; thank you. Thinking on this some more, I would argue that it we will want to split out the style from the value both to be explicit, and to separate style from data for re-use as I expect a common use case to be rendering several similar looking boxes with different titles. Consider the following:
titleStyle := lipgloss.NewStyle().
Bold(true).
Reverse(true)
mrBoxStyle := lipgloss.NewStyle().
BorderTitleStyle(borderStyle).
BorderTitle("Mr. Box").
Border(lipgloss.RoundedBorder())
mrsBoxStyle := mrBoxStyle.Copy().BorderTitle(“Mrs. Box”)
More realistically, in a view this would look something like:
boxA := boxStyle.Copy().BorderTitle(“Mr. Box”).Render(fancyData)
boxB := boxStyle.Copy().BorderTitle(“Mrs. Box”).Render(moreFancyData)
This is, of course, doable in the current implementation but a bit more roundabout:
mrTitle := lipgloss.NewStyle().
Bold(true).
Reverse(true).
SetString("Mr. Box")
mrsTitle := mrTitle.Copy().SetString(“Mrs. Box”)
mrBox := lipgloss.NewStyle().
BorderTitle(mrBox).
Border(lipgloss.RoundedBorder())
mrsBox := mrBox.Copy().BorderTitle(mrsTitle)
And in a view this would look more like:
boxA := boxStyle.BorderTitle(titleStyle.Copy().SetString(“Mr. Box”)).Render(fancyData)
boxB := boxStyle.BorderTitle(titleStyle.Copy().SetString(“Mrs. Box”)).Render(moreFancyData)
@meowgorithm Agree! This makes border styles a lot more reusable. Will rework it as you suggest.
The API is now reworked, including docs, example and readme. Hopefully I did not miss anything. @meowgorithm @maaslalani Please review.
Not 100% sure on my opinions on this but I have a feeling that if the Border is placed at the lipgloss.Right or lipgloss.Left, it should have one single horizontal (Border Top) character to the left or right of it.
Hey @eugener, we appreciate the awesome work you've put into this.
We'll play around with this now as it is and some different options we have to achieve this functionality that we were toying around with and review this PR.
Since this is a big change (changing the API), we want to make sure we're doing the correct approach so that we don't need to break it later on, so we might take a bit of extra time reviewing it.
All the suggestions are implemented. The code is now in your hands (let me know how I can help). Thank you for all the suggestions again, looking forward to see this functionality in the next release - many users will appreciate this.
Hey, thanks for your patience on this. We're still deciding what the public API should look like; the current solution in this PR includes some nested styles that we need to consider. In the meantime, we've got this gist that demonstrates how you can achieve this with the existing Lipgloss API.
https://gist.github.com/meowgorithm/1777377a43373f563476a2bcb7d89306
