Skip to content

Box should have an content Primitive#188

Closed
delaneyj wants to merge 1 commit intorivo:masterfrom
ravenops:box-contets
Closed

Box should have an content Primitive#188
delaneyj wants to merge 1 commit intorivo:masterfrom
ravenops:box-contets

Conversation

@delaneyj
Copy link
Copy Markdown
Contributor

The Box primitive is used in many examples (especially Flex) but once you start using it adding contents inside the Box is not straight forward without generating custom Primitives and .Draw() functions. This is an initial bare minimum implementation to support Box having a .SetContents(p Primitive).

This does not change how b.draw works but long term it may mean the need for a DrawFunc is unnecessary and allows for more composability.

@rivo
Copy link
Copy Markdown
Owner

rivo commented Nov 26, 2018

Thank you for your contribution. I'm not really sure I understand the motivation behind this PR. Box, while having functionality on its own, is the superclass of all other primitives. If you want to put content in a box, you don't start with Box but with one of the specialized other primitives (e.g. TextView).

I'm also not sure how your proposed changes make things easier for users of the package. If you want to put a border around a Table, you call table.SetBorders(true). No need to wrap a table in a box and then call box.SetBorders(true).

For most applications, it should not be necessary to write your own primitive. If tview is missing essential primitives in your opinion, I'd be very interested in knowing what those are.

Maybe you can describe the issues you're having (ideally with example code) so I have a better idea what you need.

@delaneyj
Copy link
Copy Markdown
Contributor Author

Ah, I see now. Makes way more sense when you explain it like that. Sorry for the confusion.

@delaneyj delaneyj closed this Nov 26, 2018
@rivo
Copy link
Copy Markdown
Owner

rivo commented Dec 3, 2018

No problem. Always happy to explain.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants