sindresorhus / emoj

Find relevant emoji from text on the command-line :open_mouth: :sparkles: :raised_hands: :horse: :boom: :see_no_evil:
MIT License
2.36k stars 57 forks source link

Improve copy option #18

Closed fleischie closed 7 years ago

fleischie commented 7 years ago

Hello. :slightly_smiling_face:

This PR is a proposal to fix #2: Check, whether there is a --copy flag given and copy the required emoji to the clipboard. For this purpose a choice parameter can be used to specify the requested index, which defaults to 1.

There are several checks in place:

Furthermore without the --copy flag but with a name parameter emoji now utilizes inquirer to display an interactive selection for the emoji to copy to the clipboard. I found this is a more usable approach to handle an ambiguous result.

NOTE: While writing this PR message I noticed there are tests in place, that fail. I will update the PR as soon as possible to fix them. You can check out the current solution in the meantime and give notes. :+1:

fleischie commented 7 years ago

Haha, the failing tests were "only" broken linting rules and I see CI enforced them. So, yeah. All :white_check_mark: now. :slightly_smiling_face:

fleischie commented 7 years ago

One of the latest commits removes the whitespace and the other now highlights the selection in color emoji terminals.

I did the initial PR on a linux machine with only emoji-font support, that allowed color-highlighting. Mac on the other hand (which I did the last two commits on) supports color emoji in it's terminal and thus needs another means of highlighting).
sindresorhus commented 7 years ago

I would like to have the same copy interface for both $ emoj and $ emoj input, so either we need to make this one horizontal, or emoj vertical. I think I prefer horizontal.

We could do something like:

❯ emoj

› horse
🐴  🐎  🏇  🎠  🚴  😍  🚌
           ^

What do you think?

fleischie commented 7 years ago

Yes, I think having a consistent layout makes sense and is preferable. 🙂

The current implementation uses inquirer and I don't know, whether it has the possibility to inquire horizontally. I will check, though.

sindresorhus commented 7 years ago

You don't need to use Inquirer. Just capture the arrow keys and move the ^ accordingly.

For example: https://github.com/sindresorhus/emoj/blob/2ea3c23667c39c61a5158f2cb815d5490d8b6de4/cli.js#L109

fleischie commented 7 years ago

Hello. I addressed the changes:

As I used the plain response from dango (and not coerce it to a string right away), this might be a breaking change from the known internals.

If you need anything, have any comments/requests/opinions/statements about this PR, just let me know. I am available for discussions/modifications of PR/etc.

sindresorhus commented 7 years ago

When I move between the emojis, the indicator (^) disappears for some milliseconds before it appears in the new position. It should not disappear.

fleischie commented 7 years ago

I addressed the indicator issues, but it seems to be not perfectly aligned in all the terminals I have. Hyper for example spaces the emojis farther away, than 3 characters (at least in my setup). iTerm2 looks good though.

Give me an update on what to address and I'll do it. Thank you for your ongoing patience. 🙇

sindresorhus commented 7 years ago

Hey, we decided to rewrite this module using Ink to make the code more readable and maintainable. This, unfortunately, makes your pull request moot. Sorry about that...

fleischie commented 7 years ago

Yeah, I already saw some days ago in my twitter feed. 😊

Thanks for taking your time, though. I'll be looking forward to the new implementation.