Add the possibility to change the orientation of Menu #9
Conversation
|
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #9 +/- ##
=====================================
Coverage 0.00% 0.00%
=====================================
Files 1 1
Lines 313 357 +44
=====================================
- Misses 313 357 +44 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
The changes in the methods add a small amount of complexity, and there's currently no tests. I wonder if you'd consider adding some to help show how this change works with simple examples. There's two parts of that
- how does the state work
- how does the menu render.
| /// | ||
| /// This is a fluent setter method which must be chained or used as it consumes self | ||
| #[must_use = "method moves the value of self and returns the modified value"] | ||
| pub const fn set_orientation(mut self, orientation: MenuOrientation) -> Self { |
There was a problem hiding this comment.
For consitency with the methods on Menu, this should probably be just orientation, not set_orientation. This pattern is the same one we use in Ratatui, and is called the builder lite pattern.
| pub const fn set_orientation(mut self, orientation: MenuOrientation) -> Self { | |
| pub const fn orientation(mut self, orientation: MenuOrientation) -> Self { |
If this is something that you need to change at runtime and not construction time, then it probably would be called set_ and take &mut self.
| if state.orientation.is_right() { | ||
| x_pos | ||
| } else { | ||
| let name_len = u16::try_from(item.name.len()).expect("name should be short"); |
There was a problem hiding this comment.
It might be difficult to actually work out what the problem is from this error message. Perhaps give more details. This is something a user will likely see, and the dev needs some info to repro.
There was a problem hiding this comment.
I'm not sure what I should do here, other than change "name" to "item name". This line just calculates the length of the name of root group, e.g. File, Edit, About in the example. I could show the problematic name in the .expect(...) but I'm not sure if this is a good idea since the length of it would be bigger than u16::MAX. I could truncate it though if that's what you want
There was a problem hiding this comment.
It seems reasonable to truncate this instead of erroring. I'm not sure I'd want to use a UI with menus that are 65536 chars long ;)
Co-authored-by: Josh McKinney <[email protected]>
src/lib.rs
Outdated
| x_pos | ||
| } else { | ||
| let name_len = u16::try_from(item.name.len()).expect("name should be short"); | ||
| x_pos - self.drop_down_width + name_len |
There was a problem hiding this comment.
x_pos is 0u16 for the first item, this substraction will panic.
There was a problem hiding this comment.
OMG, thanks for noticing this.
Unfortunately it would panic anyway since in this case the drop-down is being rendered to the left and so since there's no place for it anyway, the Clear widget will panic with this:
Trying to access position outside the buffer
I'll fix this one in a sec
src/lib.rs
Outdated
| let child_x = if orientation.is_right() { | ||
| x + self.drop_down_width | ||
| } else { | ||
| x - self.drop_down_width |
There was a problem hiding this comment.
check if x less than self.drop_down_width
|
After tried the demo, seems the "orientation" behavior should be calculated dynamically based on current layout. |
Could you elaborate? |
|
Sure. I don't have a clear design yet, just rough idea. |
It also closes #4
It's weird to have .set_orientation(...) on MenuState instead of just Menu, but since it affects MenuState::right(), MenuState::left() I think it has to be this way?
I'm also wondering whether MenuState::set_orientation(...) should take mut self or a &mut self, the second option would allow the user to adjust the orientation more easily when the program is running for example when there is not enough space on one side but there is on the other