feat: New Border Title API #97

eugener · 2022-07-29T01:56:46Z

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:

…h fg and bg colors

eugener · 2022-07-29T01:57:36Z

Once the new API is agreed upon and stable, I will also update the documentation

maaslalani · 2022-08-18T20:43:15Z

This looks really good @eugener, nice work!

eugener · 2022-09-20T18:50:10Z

@maaslalani @meowgorithm Is there anything else needed to make this PR a part the next release?

meowgorithm · 2022-09-20T18:59:30Z

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.

eugener · 2022-09-20T19:09:04Z

Sounds good! Let me know how I can help.

.gitignore

maaslalani · 2022-09-21T20:06:48Z

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())

eugener · 2022-09-21T20:20:44Z

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

maaslalani · 2022-09-21T22:06:51Z

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.

maaslalani · 2022-09-21T22:09:41Z

That being said, I am also fully onboard with the current PR as it is. I think it is currently in a shippable state.

meowgorithm · 2022-09-22T02:17:26Z

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.

…inates the need all other methods and add an ability to style the title itself.

eugener · 2022-09-22T02:41:08Z

@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.

maaslalani · 2022-09-22T03:17:11Z

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!

meowgorithm · 2022-09-22T12:44:02Z

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)

eugener · 2022-09-22T12:51:33Z

@meowgorithm Agree! This makes border styles a lot more reusable. Will rework it as you suggest.

meowgorithm · 2022-09-22T12:52:36Z

Sounds good. Thanks for all your hard work on this, @eugener!

eugener · 2022-09-22T17:57:12Z

The API is now reworked, including docs, example and readme. Hopefully I did not miss anything.
@meowgorithm @maaslalani Please review.

maaslalani

This is fantastic @eugener. Taking a look now!

maaslalani · 2022-09-22T17:58:35Z

example/main.go

@@ -239,7 +246,7 @@ func main() {

 		dialog := lipgloss.Place(width, 9,
 			lipgloss.Center, lipgloss.Center,
-			dialogBoxStyle.Render(ui),
+			dialogBoxStyle.Copy().BorderTitle(" Question ").Render(ui),


I would probably use Padding(0, 1) instead of manually adding spaces to demonstrate the BorderTitleStyle best practices.

Currently this does not work as I use actual text to calculate the required text width.
@maaslalani Do you know of any API to calculate the screen width of styled text?

I believe it might be GetHorizontalFrame to calculate all the padding and margin horizontally and then add the width of the text.

I'd additionally add logic where, if horizontal padding is unset on the style it defaults to Padding(0, 1):

╭ Title ───

However if horizontal padding is set, accept it at face value. For example, PaddingLeft(0).PaddingRight(3) would render as:

╭Title ───

maaslalani · 2022-09-22T18:01:11Z

borders.go

@@ -296,6 +324,13 @@ func (s Style) applyBorder(str string) string {
 	return out.String()
 }

+func repeatStr(s string, count int) string {


This is sort of a nitpick so feel free to ignore, but instead of having a wrapper around strings.Repeat called repeatStr I would do something like this:

strings.Repeat(border.Top, max(0, value))

Where max is:

func max(a, b int) int { if a > b { return a } return b }

You can use the max defined here:

lipgloss/style.go

Lines 460 to 465 in 0ce5550

func max(a, b int) int {

if a > b {

return a

}

return b

}

Good suggestion... coming out with the next update :)

maaslalani · 2022-09-22T18:10:07Z

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.

eugener · 2022-09-22T18:16:50Z

That is a matter of opinion :) Let's vote on this?

maaslalani · 2022-09-22T19:12:02Z

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.

Introduce default padding if one is not present.

eugener · 2022-09-22T20:02:27Z

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.

danielcmessias · 2022-10-17T12:46:18Z

Love this! looking forward to seeing it released ❤️

eugener · 2023-01-26T15:19:29Z

Is there any plan to merge this or add similar functionality?

bashbunni · 2023-03-13T15:36:18Z

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

eugener and others added 3 commits July 28, 2022 11:48

Basic implementation of the Title API

b1cac8a

Simplified way of embedding border title. Ability to render title wit…

f3ebe5c

…h fg and bg colors

Add border title alignment

2b9468b

eugener changed the title ~~Add new Border Title API~~ New Border Title API Jul 29, 2022

eugener changed the title ~~New Border Title API~~ feat: New Border Title API Jul 29, 2022

eugener mentioned this pull request Jul 31, 2022

Add ability to show block title over the border #87

Open

eugener and others added 2 commits August 19, 2022 06:07

Merge branch 'charmbracelet:master' into feature/style-title

2795dbc

Fix linting issues

346a978

Merge branch 'charmbracelet:master' into feature/style-title

80468e0

maaslalani reviewed Sep 21, 2022

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

Delete .gitignore

32c0787

Redesign BorderTitle API by converting argument to a Style. This elim…

3e6dff3

…inates the need all other methods and add an ability to style the title itself.

eugener added 4 commits September 21, 2022 23:55

Fix linting issue: remove unused getAsString function

fec27d1

Cleanup

cd06217

Fix an issue with border rendering panic.

9b317d1

Improve docs. Update README.md

80169c0

eugener and others added 3 commits September 22, 2022 11:16

Improve Border Title API according to suggestions

27c2a27

Update example code

314a2e2

Update README.md

78fea2f

maaslalani reviewed Sep 22, 2022

View reviewed changes

Remove redundant repeatStr function

0c829e8

eugener added 2 commits September 22, 2022 15:55

Calculate horizontal frame for border title correctly.

4cf8c19

Introduce default padding if one is not present.

Update example

9e516bc

muesli added the enhancement New feature or request label Oct 5, 2022

Merge branch 'charmbracelet:master' into feature/style-title

d59ed63

MHNightCat mentioned this pull request Apr 29, 2024

feat: Update new border info api (show information on bottom border) #284

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: New Border Title API #97

feat: New Border Title API #97

eugener commented Jul 29, 2022 •

edited

eugener commented Jul 29, 2022

maaslalani commented Aug 18, 2022

eugener commented Sep 20, 2022

meowgorithm commented Sep 20, 2022

eugener commented Sep 20, 2022

maaslalani commented Sep 21, 2022 •

edited

eugener commented Sep 21, 2022 •

edited

maaslalani commented Sep 21, 2022 •

edited

maaslalani commented Sep 21, 2022

meowgorithm commented Sep 22, 2022 •

edited

eugener commented Sep 22, 2022 •

edited

maaslalani commented Sep 22, 2022 •

edited

meowgorithm commented Sep 22, 2022 •

edited

eugener commented Sep 22, 2022

meowgorithm commented Sep 22, 2022

eugener commented Sep 22, 2022

maaslalani left a comment

maaslalani Sep 22, 2022

eugener Sep 22, 2022

maaslalani Sep 22, 2022

meowgorithm Sep 22, 2022 •

edited

maaslalani Sep 22, 2022 •

edited

maaslalani Sep 22, 2022

eugener Sep 22, 2022

maaslalani commented Sep 22, 2022 •

edited

eugener commented Sep 22, 2022

maaslalani commented Sep 22, 2022 •

edited

eugener commented Sep 22, 2022

danielcmessias commented Oct 17, 2022

eugener commented Jan 26, 2023

bashbunni commented Mar 13, 2023

feat: New Border Title API #97

Are you sure you want to change the base?

feat: New Border Title API #97

Conversation

eugener commented Jul 29, 2022 • edited

eugener commented Jul 29, 2022

maaslalani commented Aug 18, 2022

eugener commented Sep 20, 2022

meowgorithm commented Sep 20, 2022

eugener commented Sep 20, 2022

maaslalani commented Sep 21, 2022 • edited

eugener commented Sep 21, 2022 • edited

maaslalani commented Sep 21, 2022 • edited

maaslalani commented Sep 21, 2022

meowgorithm commented Sep 22, 2022 • edited

eugener commented Sep 22, 2022 • edited

maaslalani commented Sep 22, 2022 • edited

meowgorithm commented Sep 22, 2022 • edited

eugener commented Sep 22, 2022

meowgorithm commented Sep 22, 2022

eugener commented Sep 22, 2022

maaslalani left a comment

Choose a reason for hiding this comment

maaslalani Sep 22, 2022

Choose a reason for hiding this comment

eugener Sep 22, 2022

Choose a reason for hiding this comment

maaslalani Sep 22, 2022

Choose a reason for hiding this comment

meowgorithm Sep 22, 2022 • edited

Choose a reason for hiding this comment

maaslalani Sep 22, 2022 • edited

Choose a reason for hiding this comment

maaslalani Sep 22, 2022

Choose a reason for hiding this comment

eugener Sep 22, 2022

Choose a reason for hiding this comment

maaslalani commented Sep 22, 2022 • edited

eugener commented Sep 22, 2022

maaslalani commented Sep 22, 2022 • edited

eugener commented Sep 22, 2022

danielcmessias commented Oct 17, 2022

eugener commented Jan 26, 2023

bashbunni commented Mar 13, 2023

eugener commented Jul 29, 2022 •

edited

maaslalani commented Sep 21, 2022 •

edited

eugener commented Sep 21, 2022 •

edited

maaslalani commented Sep 21, 2022 •

edited

meowgorithm commented Sep 22, 2022 •

edited

eugener commented Sep 22, 2022 •

edited

maaslalani commented Sep 22, 2022 •

edited

meowgorithm commented Sep 22, 2022 •

edited

meowgorithm Sep 22, 2022 •

edited

maaslalani Sep 22, 2022 •

edited

maaslalani commented Sep 22, 2022 •

edited

maaslalani commented Sep 22, 2022 •

edited