Toggle UI split #287
Toggle UI split #287
Conversation
Byron
left a comment
Byron
left a comment
There was a problem hiding this comment.
Thanks a lot!
I think the PR looks simple enough, and once CI is fixed there should also be an entry in the help menu to document the new shortcut.
I am not sure if the help-text should be even longer for something that is optional to the typical workflow.
Thanks again for your help getting this ready.
|
@Byron I made the changes but am having a hard time understanding why the CI is failing. Would you kindly shed some light there for me? |
There was a problem hiding this comment.
Thanks! This time I tried it and it works as advertised, it's definitely good to have such 'pane-rotation'.
It looks like CI is generally broken now for some reason and I wonder if @joehasson has more intuition than me.
Disregarding CI, I have a few more comments that would need addressing.
Thanks again.
|
|
||
| fn draw_bottom_right_help(bound: Rect, buf: &mut Buffer) { | ||
| let bound = line_bound(bound, bound.height.saturating_sub(1) as usize); | ||
| let help_text = " mark-move = d | mark-toggle = space | toggle-all = a "; |
There was a problem hiding this comment.
The trailing space is needed for… spacing :).
src/interactive/widgets/help.rs
Outdated
| ); | ||
| hotkey("<Space>", "Toggle the currently selected entry.", None); | ||
| hotkey("a", "Toggle all entries.", None); | ||
| hotkey("u", "Toggle UI split Vertical/Horizontal.", None); |
src/interactive/app/eventloop.rs
Outdated
| return Ok(Some(result?)); | ||
| } | ||
| } | ||
| Char('u') => { |
There was a problem hiding this comment.
uh, do you have any suggestions in lieu of "u" ? Thoughts on upper case "U" ?
There was a problem hiding this comment.
I think you will have to try out what feels right on an english keyboard.
|
@Byron Regarding CI, I actually don't think it is broken generally: I've been able to reproduce the failing tests on linux locally by bisecting to the changes introduced in commit 19e1e17 of this PR (I don't have a windows machine to try on but I imagine the culprit is the same). Its not clear to me why these changes would cause the tests to fail though. |
|
Thanks for debugging CI, that will certainly be useful when trying to fix i! Interestingly, I definitely merge a failed CI, so that must have happened sneakily. I'd merge this PR without green CI though as CI is unrelated, but am waiting for the other comments to be addressed. |
|
Closing due to inactivity. Please feel free to reopen or resubmit. Thanks for your understanding. |
3 participants
Hi @Byron,
I added a feature to toggle the UI direction because that's useful for long file names in small monitors.
Thanks for making dua-cli!