-
Notifications
You must be signed in to change notification settings - Fork 258
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
fix(block): make inner
aware of title positions
#657
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #657 +/- ##
=====================================
Coverage 90.7% 90.7%
=====================================
Files 42 42
Lines 12170 12249 +79
=====================================
+ Hits 11042 11121 +79
Misses 1128 1128 ☔ View full report in Codecov by Sentry. |
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.
Can you also add a test case where you cover the issue in #658? (you can assert the contents of the buffer with assert_buffer
)
Good suggestion. I have amended the PR accordingly. Let me know if anything can be improved. |
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.
Can you please suggest a simple and concise explanation of the bug?
If you're able, add it to the body of a commit message to make it automatically fill the changelog. If not, we can add the wording when squashing the commit.
I have added an explanation of the bug and the fix to both the description of this PR and the commit message. Let me know should brevity be lacking, or if the level of detail could be improved upon (in either direction). Also, I have reworked the part that @joshka had a nit about, without accepting the suggestion. If you prefer to have your suggestion exactly, I'll incorporate that instead. |
Possible more brevity would be helpful (to avoid fatigue when reading a changelog), but what you have is also fine if you'd like it. Happy to merge as-is or take your changes if you want to put them. Previously, when computing the inner rendering area of a block, all |
Previously, when computing the inner rendering area of a block, all titles were assumed to be positioned at the top, which caused the height of the inner area to be miscalculated.
Thanks for the shortened bug description as well as the improved code snippet. 👍 I've incorporated both and amended the PR accordingly. |
Thanks for your contribution! |
Thanks for the guidance to get this merged smoothly. 🙂 |
Previously, when computing the inner rendering area of a block, all titles were assumed to be positioned at the top. If some or all titles were at the bottom, this caused various incorrect computations of the inner area, depending on which borders were set; no border at the bottom would lead to an area that's too big, no border at the top but at the bottom and all titles at the bottom would lead to an area that's too small.
Now, when computing the inner area, the positions of the titles are taken into account.
fix #658