Added feature to align title of the block #462
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.
Sorry for keeping you waiting for a review. This is a very nice pull request. I have some suggestions to make it even better. Let me know what you think :).
src/widgets/block.rs
Outdated
|
||
let title_dx = match self.title_alignment { | ||
Alignment::Left => 0, | ||
Alignment::Center => (title_area_width - title.width() as u16) / 2, |
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.
This will probably panic if title_area_width < title.width()
. You may want to use saturating_sub
here as well unless the precondition is checked beforehand. This could use a test as well.
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 will update to use saturating_sub
and add a test for this case. Thank you for pointing this out.
@@ -507,5 +532,39 @@ mod tests { | |||
height: 0, | |||
}, | |||
); | |||
assert_eq!( |
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.
The title_alignment
is not taken into account in Block::inner
. Therefore, is there a reason to add those test cases here ?
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 added them to show that title_alignment
does not effect Block::inner
. I am not sure, if that is necessary.
tests/widgets_block.rs
Outdated
@@ -211,3 +211,471 @@ fn widgets_block_renders_on_small_areas() { | |||
Buffer::with_lines(vec!["┌Test─"]), | |||
); | |||
} | |||
|
|||
#[test] | |||
fn widgets_block_renders_title_top_left_all_borders() { |
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 including tests. However, a lot of them share the same structure. Could we use a more data-driven approach here ? For example, in widgets_paragraph_can_wrap_its_content, the common structure is extracted to a closure which is then called with all test inputs. Additionally, the TestBackend
could maybe a bit smaller to have more concise buffer assertions.
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.
Ohhhh, that is so neat! Thank you for showing the closure example, I will try to do something similar.
I will make TestBackend
smaller.
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.
Thank you for all of the suggestions :)
@@ -507,5 +532,39 @@ mod tests { | |||
height: 0, | |||
}, | |||
); | |||
assert_eq!( |
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 added them to show that title_alignment
does not effect Block::inner
. I am not sure, if that is necessary.
src/widgets/block.rs
Outdated
|
||
let title_dx = match self.title_alignment { | ||
Alignment::Left => 0, | ||
Alignment::Center => (title_area_width - title.width() as u16) / 2, |
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 will update to use saturating_sub
and add a test for this case. Thank you for pointing this out.
tests/widgets_block.rs
Outdated
@@ -211,3 +211,471 @@ fn widgets_block_renders_on_small_areas() { | |||
Buffer::with_lines(vec!["┌Test─"]), | |||
); | |||
} | |||
|
|||
#[test] | |||
fn widgets_block_renders_title_top_left_all_borders() { |
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.
Ohhhh, that is so neat! Thank you for showing the closure example, I will try to do something similar.
I will make TestBackend
smaller.
…net code to be more straightforward and have correct behavior when placing title in the center without left border
Committed updates based on the suggestions and fixed behavior of the center alignment when used without left border. |
This is PR for #450
For all above, used layout::Alignment enum as the type for title_alignment.