Fix incorrect panel height calculation in complex layout #1514
Conversation
|
@patriksvensson The only major modification I was considering was a bounds check to see if the total height could go over the target, do you think that is needed? |
|
I'll review this @BlazeFace |
FrankRay78
changed the title
|
@FrankRay78 if we are looking to merge this I can go ahead and rebase and resolve conflicts. |
|
I would like to merge this. A few minor nits to add to a review, which I will do soon, but definitely go ahead and rebase please. |
# Conflicts: # src/Tests/Spectre.Console.Tests/Expectations/Widgets/Layout/Render_Layout_With_Three_And_One_Columns.Output.verified.txt
|
@FrankRay78 Rebase complete |
| @@ -264,4 +264,37 @@ public Task Should_Fall_Back_To_Parent_Layout_If_All_Children_Are_Invisible() | |||
| // Then | |||
| return Verifier.Verify(console.Output); | |||
| } | |||
|
|
|||
| [Fact] | |||
There was a problem hiding this comment.
Should we make this a [Theory] and pass in the problematic console heights 17, 20, 23, 28, 31 ?
| @@ -264,4 +264,37 @@ public Task Should_Fall_Back_To_Parent_Layout_If_All_Children_Are_Invisible() | |||
| // Then | |||
| return Verifier.Verify(console.Output); | |||
| } | |||
|
|
|||
| [Fact] | |||
| [Expectation("Render_Layout_With_Three_And_One_Columns")] | |||
There was a problem hiding this comment.
I think this/the underlying expectation file should probably be renamed to Render_Layout_With_Nested_Three_Rows_In_One_Column, to be more consistent with:
|
|
||
| [Fact] | ||
| [Expectation("Render_Layout_With_Three_And_One_Columns")] | ||
| public Task Should_Render_Layout_With_Three_And_One_Columns() |
There was a problem hiding this comment.
Could you please move this to line 156, so your test follows directly after public Task Should_Render_Layout_With_Nested_Rows_And_Columns(), which is a natural grouping.
| foreach (var l in layouts) | ||
| { | ||
| l.Update(panel); | ||
| } |
There was a problem hiding this comment.
I've thought about this, and whilst I'm happy with the correctness of the above test, all the other tests in the same file use a fluent convention. Can you please have a go at updating your test to be in the same style as the others, here's a close example to follow (I'm sorry for this 🏈 ache request, and I know the test comes direct from the issue repro code):
public Task Should_Render_Layout_With_Nested_Rows_And_Columns()
{
// Given
var console = new TestConsole().Size(new Size(40, 15));
var layout = new Layout()
.SplitRows(
new Layout("Top")
.SplitRows(
new Layout("T1")
.SplitColumns(
new Layout("A"),
new Layout("B")),
new Layout("T2")
.SplitColumns(
new Layout("C"),
new Layout("D"))),
new Layout("Bottom")
.SplitRows(
new Layout("B1")
.SplitColumns(
new Layout("E"),
new Layout("F")),
new Layout("B2")
.SplitColumns(
new Layout("G"),
new Layout("H"))));
// When
console.Write(layout);
// Then
return Verifier.Verify(console.Output);
}
There was a problem hiding this comment.
Hello @BlazeFace, if you'll be kind enough to entertain my review feedback, I'll be most kind enough to merge the PR once done. (So you know your efforts won't languish in an unmerged PR).
|
Thanks @BlazeFace, I've seen lots of activity but not had the time to look just yet. But I will do so soon. |
|
Good stuff @BlazeFace. Don't hesitate to tag me when you have other PRs to review. |
|
@FrankRay78 I think you forgot to approve and merge 😉 |
|
A teacher knows to be humble before the master. |
patriksvensson
dismissed
FrankRay78’s stale review
Everything in the review has been fixed, so dismissing it so the PR can be merged.
|
Merged! Thank you for your contribution. Much appreciated! 👍 |
fixes #1390
Changes
Updated the ratio calculation to round up if the remainder is within 0.9999 I have tested this with a good amount of examples and it still works, but I have not written any proof that this will remain correct at any value. We could use a much smaller target to round say .99 as well.
Please upvote 👍 this pull request if you are interested in it.