Fix interactive prompt for options > terminal rows#35
Conversation
lib/cli/ui.rb
Outdated
| # | ||
| # * +enable_color+ - should color be used? default to true | ||
| # | ||
| def self.fmt_full_line(input, enable_color: true) |
There was a problem hiding this comment.
We should be able to accomplish the same thing by writing something like:
\e[nA or \e[nB # to move to the right row
\r # to reset to col 0
<text>
\e[K # to clear to the end of the line
This would be flicker-free too.
EDIT: e.g.: puts "asdf asdf asdf asdf \n\e[A\rasdf\e[K\n"
burke
left a comment
burke
left a comment
There was a problem hiding this comment.
Mostly LGTM but let's use \e[K rather than padding
|
@burke: I did this without |
lugray
force-pushed
the
handle_many_options
branch
3 times, most recently
from
1360fa1 to
98e4133
Compare
| end | ||
| end | ||
|
|
||
| def presented_options(recalculate: false) |
There was a problem hiding this comment.
this is really hard to follow.
I feel like it would do well to have some comments throughout explaining the code as it's quite logic dense and sort of magic with the [-2].last etc
|
Refactored to hopefully improve clarity. |
| def presented_options(recalculate: false) | ||
| return @presented_options unless recalculate | ||
|
|
||
| @presented_options = @options.zip(1..Float::INFINITY) |
There was a problem hiding this comment.
@options.each.with_index(1) is another way if you think it's more clear. I'm undecided.
There was a problem hiding this comment.
This change would need to_a as well (as you suggest is an emumerator which prevents the manipulations below). The zip seems clearer to me.
|
|
||
| @presented_options = @options.zip(1..Float::INFINITY) | ||
| while num_lines > max_options | ||
| if last_visible_option_number - @active > @active - first_visible_option_number |
There was a problem hiding this comment.
I agree that this is kind of hard to read. Maybe change to:
distance_from_selection_to_end = last_visible_option_number - @active
distance_from_start_to_selection = @active - first_visible_option_number
# try to keep the selection centered in the window:
if distance_from_selection_to_end > distance_from_start_to_selection
# selection is closer to top than bottom, so trim a row from the bottom
...
else
# closer to bottom, so trim from top
lib/cli/ui/terminal.rb
Outdated
| module UI | ||
| module Terminal | ||
| DEFAULT_WIDTH = 80 | ||
| DEFAULT_HEIGHT = 5 |
There was a problem hiding this comment.
The typical default height is 24 fwiw
burke
left a comment
burke
left a comment
There was a problem hiding this comment.
LGTM. I still think I like my naming for the dense bit a little better but no major complaint.
Implement a scrolling list
lugray
force-pushed
the
handle_many_options
branch
from
038d9bd to
32ef33b
Compare
|
Updated. |
4 participants
Implement a scrolling list.
wait_for_actionable_user_inputstops re-rendering options when not necessary (midway through processing a ⬆️/⬇️ sequence) since it was causing some noticeable flicker, but also means that tests had to change more, since fewer lines are written.fmt_full_lineis needed now since the lengths of lines can change, which was not true before since the only thing that changed was the marker vs space and colour. This also caused tests to change, prompting theoption_texthelper.cc @Shopify/dev-infra
Fixes #34