-
Notifications
You must be signed in to change notification settings - Fork 205
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
Feature: Tables #218
Feature: Tables #218
Conversation
I think we should have a "DefaultStyle" function that returns some basic styling for a table Also, I think when setting headers and rows we should accept |
I think we also need to add some kind of restrictions or a check to make sure that we don't try to index out of range when iterating over the cells in the rows since there is the possibility right now that there are more cells in a row than headings in the header. |
Note to self:
|
This has been great to work with btw Maas! |
Thanks so much @bashbunni! |
BorderStyle(re.NewStyle().Foreground(lipgloss.Color("238"))). | ||
Headers(CapitalizeHeaders(headers)...). | ||
Rows(data...). | ||
StyleFunc(func(row, col int) lipgloss.Style { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@maaslalani I think StyleFunc
should be named something else to indicate that it's for cell styling. It's a bit vague imo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah gotcha, good to know! Thank you :)
table/rows.go
Outdated
// Data is the interface that wraps the basic methods of a table model. | ||
type Data interface { | ||
Row(row int) Row | ||
Append(row Row) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think Append
should remain optional. Not all data models can/will support that.
table/table.go
Outdated
} | ||
|
||
// Rows sets the table data. | ||
func (t *Table) Rows(data Data) *Table { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think Rows(data Data)
is even weirder then Data(data Data)
. Also comparing Row
with Rows
, I would expect them to accept the same type, just singular vs plural?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I'm trying to say: this function sets an entire data model, and hence the names should probably match. In addition to Row
we can then have Rows
, which lets you set multiple string rows at once.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep you're right! I will change this.
I'll keep the Rows method and then also add a Data method.
e881821
to
1d71b7c
Compare
table/table.go
Outdated
case *StringData: | ||
t.data.(*StringData).Append(StringRow(row)) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@muesli does this seem reasonable to you? This is how we would avoid Append
being a required message, but not sure if this is the correct way to do it.
ac00ad4
to
6a014e9
Compare
Adds
NewTable
+ features to Lip Gloss.API:
Results: