konradsz / igrep

Interactive Grep
MIT License
663 stars 17 forks source link

add a keybindings popup #52

Closed TeFiLeDo closed 1 year ago

TeFiLeDo commented 1 year ago

Resolves #47.

Currently the content of the popup is just a paragraph. It might be worthwhile to change this, so that it uses the Table widget. However, to do this I'd need a bit more time, as I currently don't really understand how that would work.

I'm also thinking about adding a build script to extract the keybindings table from the README.md, so the information doesn't get out of sync.

I added the popup shortcuts at the top to give them the best chance of being visible in a small terminal.

konradsz commented 1 year ago

I am impressed, thank you 🥇

I have following ideas in mind (sorted by "priority"):

Build script for extracting the text from README sounds great, but you can always do it in a separate PR if you fancy for a challenge :)

Let me know if you have time and interest in applying my suggestions, I can always merge it as is (cause it does its job!) and do it in the future.

TeFiLeDo commented 1 year ago
  1. Added the "any key to close" behavior. Should that line specify that scrolling keys aren't affected?
  2. The popup now uses as much space as it needs. It still is capped at 80% on small terminals, so that it is always identifiable as a popup.
  3. I added the the igrep v1.2.0 to the title of the popup. I don't think it looks good as part of the popup content. Please see if this is to your liking.
  4. Extraction via build script is now implemented.
konradsz commented 1 year ago

There is one bug in your scrolling implementation. If you scroll down to the very bottom and then press "j" or "down" a number of times, then you need to press "k" or "up" the same number of times before it starts scrolling back :)

The rest looks great!

EDIT: I would not mention that scrolling keys do not close the popup.

TeFiLeDo commented 1 year ago

Scrolling should be fixed now