-
Notifications
You must be signed in to change notification settings - Fork 27
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
Add minimum height option to table #154
Conversation
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.
Thanks for the effort, and the test coverage! Seems reasonable overall, just a comment about modifying that exported function.
Would be good to slip this into one of the examples as well. Maybe the flex
example since it's dealing with stretching horizontally, we can also stretch it vertically.
Sounds good, I've added it to the flex example! |
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.
LGTM
Ah we're going to need to be careful with coverage as well. You've added a bunch of checks in the options, but looks like they aren't covered: https://coveralls.io/builds/63298625/source?filename=table%2Foptions.go I know it's a bit verbose, but it would be helpful to have tests to make sure the expected internal height values are correct after adding these options when a minimum height is set. |
Actually, looking at your tests, I think we'd just have to change the order of things. Basically, apply the minimum height first, then apply the options after. This would call your checks directly and should help considerably. Maybe pass in a function that applies extra options rather than a pre-set model with the options already applied? |
Do you mean in the view tests? What do you think about adding a test in |
I think that would work fine too! |
Thanks for the contribution! Will release this along with your other PR once it's in. |
Fixes #115.
Adds a
WithMinimumHeight()
option toModel
. When set, we add "padding" rows when rendering the view to meet the specified minimum height.Caveats: