Add search command (WIP) #11

chrisforrette · 2018-11-07T23:45:00Z

This is a work-in-progress PR to add an interactive search command to tink.

Usage:

./bin/tink.js search: Fully interactive package search
./bin/tink.js search "some search term": Auto-execute a search for the passed in terms.

Added dependencies:

Todos:

Iron out input focus. There is currently a focus conflict between the text input for search terms and the select input for selecting matched packages for install in that they are both focused on, and when you hit "Enter" in the latter, both of them receive a submit action.
Handle CLI arguments. Currently all options from lib/common-opts.js are being passed into the Search component but aren't addressed at all. We may not need all/any of those options, and we should add options to be passable to NPM search.
Integrate PackageView. This is being built by @larsgw and might happen after this PR, and would display package details when highlighting packages in search results.

chrisforrette · 2018-11-07T23:53:20Z

lib/commands/search.jsx

+    return y.help().alias('help', 'h')
+      .options(SearchCommand.options)
+  },
+  options: Object.assign(require('../common-opts.js'), {}),


@zkat What you were saying over here makes sense about passing the whole options object to libnpm.search—so just to confirm, I don't need common-opts.js at all for the search command, right? So I can change this to a figgy-pudding object with all the libnpmsearch opts?

bin/tink.js

lib/commands/search.jsx

lib/components/search.jsx

package.json

zkat · 2018-11-07T23:56:03Z

lib/components/search.jsx

+  return <div>
+    <Color grey bold>Find a package: </Color>
+    <TextInput value={terms} onChange={onChange} onSubmit={onSubmit} />
+  </div>


For some reason, if I erase the textinput data and press "Enter" it does BOTH a search and an install (and closes the process). Or was I testing an older version?

Yea! This is that weird input focus thing I mentioned in my Todos list. I saw that the ink-select-input has an explicit focus boolean prop, and I just realized ink-text-input has the same prop undocumented. I just need to add some logic to juggle these intuitively...

legodude17

So excited for this! 🎉

lib/commands/search.jsx

legodude17 · 2018-11-08T18:23:08Z

lib/components/search.jsx

+const libnpm = require('libnpm')
+
+
+const MAX_RESULTS = 10


Shouldn't this be configurable?

I don't really care to make something like this configurable. Just pick good defaults imo

In a separate thread I was talking about changing the yargs options to pass down all the options of libnpmsearch, so this will just change to the default for the limit option, but allow it to be overridden by the user.

lib/components/search.jsx

lib/commands/search.jsx

zkat · 2018-11-11T17:56:30Z

FYI you'll need to accept #22 before I can merge this.

chrisforrette · 2018-11-11T20:37:58Z

@zkat Done!

chrisforrette · 2018-11-11T20:38:43Z

@zkat BTW, hoping to get to these changes this evening, sorry I've been a little slow...

zkat · 2018-11-12T02:28:27Z

You're fine! Take your time. We're not gonna be launching this until like next year so just take care of yourself first and foremost, and then if you have some time left over, go ahead and hack for fun <3

chrisforrette · 2018-11-14T07:24:25Z

@zkat Got a few updates besides the ones I've already commented on...

I've added an options object to the search command which is basically all of the libnpmsearch options, which are now being passed to the search call.

I am also trying to mimic your comment style now 😄 (I saw the CONTRIBUTING guide too late). Let me know if you'd like me go back and modify my commit messages.

Next I'm diving into that input focus thing—I have a good idea of user flow and might need to get a little hacky with it—and integrating the PackageView.

zkat · 2018-11-19T19:43:39Z

idea: Can we render things like a table? I think aligning the various elements will make it a lot more readable.

larsgw · 2018-11-19T21:54:00Z

I can make a PackageSummary to avoid obscuring the search select input and it jumping around given the relatively high and variable height of PackageView.

Maybe another command would be cool too, a full-screen (like htop/tig/man) terminal remake of the site. But that's definitely out of scope for this PR.

chrisforrette · 2018-11-21T21:33:02Z

@zkat Yea, I think rendering results in a table is a great idea. I'll see if I can compose components from ink-table and npm-select-input together to pull it off, but it might involve a custom franken-component.

@larsgw That sounds good—I assume you're thinking like a one-liner? I was originally thinking of putting it at the bottom of the matches list, but people might miss it if there are a lot of results.

zkat · 2018-11-29T21:15:06Z

How's it going with this?

chrisforrette · 2018-11-29T22:08:20Z

@zkat Good! Sorry for the radio silence.

I've been ironing out different states of searching to get them feeling right, and also being distracted by holiday busy-ness and such. After that I'm going to take a stab at displaying matching packages in a table view, but I'm thinking if it's less-than-straightforward, it might be better to put that into a follow-up PR if that's alright with you.

Same goes for the one-liner package view (displaying package info on one line when you highlight it). I'm not sure if this is something @larsgw is actively working on, or that I could/should work on, but it might be simpler in a separate PR.

larsgw · 2018-11-29T22:17:53Z

I haven't started on it yet, sorry for that. I was thinking of 3 lines of content, with maybe 2-3 lines of visual distinction (I like this one), but that's up to you.

chrisforrette · 2018-11-30T18:51:05Z

@larsgw No worries, you're not blocking me or anything. Let me know if you get started on it, and I'll do the same if I get to it first. Thanks!

larsgw · 2018-12-01T23:28:42Z

Just to make sure we don't do the same work twice: I made PR #38

chrisforrette · 2018-12-03T02:39:05Z

@larsgw Awesome, thanks!

chrisforrette · 2018-12-04T06:52:33Z

@zkat OK, I think this is ready for another look!

I think I ironed out the input focus stuff—I did a few tests with and without an initial term search, then deleting and entering a new search and it's feeling pretty good.

And I looked into using ink-table and it isn't a simple drop-in to use it for the selector. There's some pretty unique logic to render out the table, so I think to pull it off we would indeed need to make a franken-component out of it and ink-select-input. I'm down to build it, but I'd prefer to do it in a follow-up PR if that's cool.

And once #38 is good to go, I can drop it in over here.

Let me know what you think, thanks!

zkat · 2018-12-10T15:57:04Z

This looks great so far!

chrisforrette · 2018-12-10T17:44:19Z

@zkat Awesome, thanks!

For getting this merged, do you want the table display and the one-liner package description in place first?

zkat · 2018-12-30T06:11:29Z

@chrisforrette oh geez sorry it took so long to notice this again! No, I don't think table display is necessary yet, but I'd like to see more of a summary for each displayed package.

chrisforrette · 2019-01-05T21:06:19Z

@zkat No worries! I'll implement the PackageSearchResult component from @larsgw for the package summary once #38 is merged.

…and flow, installing a few Ink components for text input and package selection

…ion import

…ad without it

…ent instead of an object

…ext and tweaked format of package scores

…rch options and passing them through to the search query call

…ator and spinner

…ect input entirely and conditionally rendering it instead

…ay alongside the 'installing...' message

…n search input should be edited

…removing JSX from command itself, changing search results state a bit to not display blank results when terms are deleted

chrisforrette · 2019-01-05T21:32:52Z

@zkat Btw, is there a real package installer I should use yet? I noticed lib/installer.js but it looks like that's installing everything from package.json. Should I add an addPackage method in there or something?

SchneiderOr · 2019-01-28T18:38:03Z

Hey guys any news about this feature? Would love to assist if any further help is needed

chrisforrette · 2019-01-31T18:26:19Z

@SchneiderOr I was waiting on #38 to implement here, I'll go nudge over there...

chrisforrette · 2019-01-31T18:27:03Z

@zkat Also I'm not sure if you saw my last comment, but is there a real install function I should wire up to this? Or should it remain nerfed?

chrisforrette commented Nov 7, 2018

View reviewed changes

zkat suggested changes Nov 7, 2018

View reviewed changes

legodude17 reviewed Nov 8, 2018

View reviewed changes

chrisforrette force-pushed the feature/search-command branch from ef5c1c9 to 48bac49 Compare November 14, 2018 06:14

larsgw mentioned this pull request Nov 30, 2018

feat(view): add PackageSummary #38

Open

2 tasks

chrisforrette force-pushed the feature/search-command branch 2 times, most recently from ea8b0b9 to e7ea10d Compare December 4, 2018 06:23

chrisforrette added 4 commits January 5, 2019 13:07

Adding basic foundation for search command

d390a5c

Trying to JSX the search

da1c070

Fleshing out a bunch of the interactive search component composition …

aea8899

…and flow, installing a few Ink components for text input and package selection

Getting fake install flow working

799c11d

chrisforrette added 17 commits January 5, 2019 13:20

Customizing package listing details and select components

fc84e7c

Fixing inline search term search

7204636

deps: Adding [email protected]

a009788

fix(search): making search command JSX-ish and removing ink 'h' funct…

4a0bd88

…ion import

fix(search): adding ink 'h' import back to search command, ink gets m…

64c7e71

…ad without it

fix(search): changing search component export to be the search compon…

0588526

…ent instead of an object

fix(search): added maintenance/popularity/quality legend to results t…

4dca41e

…ext and tweaked format of package scores

feat(search): adding options config object with almost all libnpm sea…

c45cbbd

…rch options and passing them through to the search query call

fix(search): removing special character stuff: package selector indic…

096c2a0

…ator and spinner

deps: removing figures and ink-spinner

6842d04

feat(search): worked out a decent control flow, removing focus on sel…

989b967

…ect input entirely and conditionally rendering it instead

feat(search): making it so that the search input prompt doesn't displ…

ead33b5

…ay alongside the 'installing...' message

feat(search): fixing issue with focus remaining on search results whe…

c521af8

…n search input should be edited

feat(search): linting

45fa41a

feat(search): removing stray todo note

ff28c51

feat(search): updating search command, moving it into yargs-modules, …

6e583e9

…removing JSX from command itself, changing search results state a bit to not display blank results when terms are deleted

Updating package-lock.json, removing unnecessary eslint comment

60091f5

chrisforrette force-pushed the feature/search-command branch from e7ea10d to 60091f5 Compare January 5, 2019 21:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add search command (WIP) #11

Add search command (WIP) #11

chrisforrette commented Nov 7, 2018 •

edited

Loading

chrisforrette Nov 7, 2018

zkat Nov 7, 2018

chrisforrette Nov 8, 2018

legodude17 left a comment

legodude17 Nov 8, 2018

zkat Nov 8, 2018

chrisforrette Nov 11, 2018

zkat commented Nov 11, 2018

chrisforrette commented Nov 11, 2018

chrisforrette commented Nov 11, 2018

zkat commented Nov 12, 2018

chrisforrette commented Nov 14, 2018

zkat commented Nov 19, 2018

larsgw commented Nov 19, 2018

chrisforrette commented Nov 21, 2018

zkat commented Nov 29, 2018

chrisforrette commented Nov 29, 2018

larsgw commented Nov 29, 2018 •

edited

Loading

chrisforrette commented Nov 30, 2018

larsgw commented Dec 1, 2018 •

edited

Loading

chrisforrette commented Dec 3, 2018

chrisforrette commented Dec 4, 2018

zkat commented Dec 10, 2018

chrisforrette commented Dec 10, 2018

zkat commented Dec 30, 2018

chrisforrette commented Jan 5, 2019

chrisforrette commented Jan 5, 2019

SchneiderOr commented Jan 28, 2019

chrisforrette commented Jan 31, 2019

chrisforrette commented Jan 31, 2019

Add search command (WIP) #11

Are you sure you want to change the base?

Add search command (WIP) #11

Conversation

chrisforrette commented Nov 7, 2018 • edited Loading

chrisforrette Nov 7, 2018

Choose a reason for hiding this comment

zkat Nov 7, 2018

Choose a reason for hiding this comment

chrisforrette Nov 8, 2018

Choose a reason for hiding this comment

legodude17 left a comment

Choose a reason for hiding this comment

legodude17 Nov 8, 2018

Choose a reason for hiding this comment

zkat Nov 8, 2018

Choose a reason for hiding this comment

chrisforrette Nov 11, 2018

Choose a reason for hiding this comment

zkat commented Nov 11, 2018

chrisforrette commented Nov 11, 2018

chrisforrette commented Nov 11, 2018

zkat commented Nov 12, 2018

chrisforrette commented Nov 14, 2018

zkat commented Nov 19, 2018

larsgw commented Nov 19, 2018

chrisforrette commented Nov 21, 2018

zkat commented Nov 29, 2018

chrisforrette commented Nov 29, 2018

larsgw commented Nov 29, 2018 • edited Loading

chrisforrette commented Nov 30, 2018

larsgw commented Dec 1, 2018 • edited Loading

chrisforrette commented Dec 3, 2018

chrisforrette commented Dec 4, 2018

zkat commented Dec 10, 2018

chrisforrette commented Dec 10, 2018

zkat commented Dec 30, 2018

chrisforrette commented Jan 5, 2019

chrisforrette commented Jan 5, 2019

SchneiderOr commented Jan 28, 2019

chrisforrette commented Jan 31, 2019

chrisforrette commented Jan 31, 2019

chrisforrette commented Nov 7, 2018 •

edited

Loading

larsgw commented Nov 29, 2018 •

edited

Loading

larsgw commented Dec 1, 2018 •

edited

Loading