-
Notifications
You must be signed in to change notification settings - Fork 131
feat(core + prompts): adds autocomplete #288
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
🦋 Changeset detectedLatest commit: 984a92d The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@example/basic • @example/changesets
commit: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the best option to handle switching between navigation and filtering mode is to expose the _track
property of the Prompt
class.
In navigation _track
is false
, and true
when filtering.
The value
event is only triggered when _track
is true
,
so if _track
is false
we don't have to worry about space being added in the value.
But in exchange, we have to readd the key that change the mode from navigation to filtering
This PR will replace #124 |
} | ||
|
||
export const limitOptions = <TOption>(params: LimitOptionsParams<TOption>): string[] => { | ||
const { cursor, options, style } = params; | ||
const output: Writable = params.output ?? process.stdout; | ||
const rows = output instanceof WriteStream && output.rows !== undefined ? output.rows : 10; | ||
const overflowFormat = params.overflowFormat ?? color.dim(' ...'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did we ever use this? it looks like the only places we set it, we use the default
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm not sure about this one tbh.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looking at the snapshots, it seems this changes how some of the existing prompts render
so maybe we should just revert this bit as it seems unnecessary for the autocomplete
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we maybe revert this @dreyfus92?
unless you can figure out why it exists
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will try to dive into this tmr 🫡, this might be the last piece right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@43081j, I've noticed that overflowFormat
param is used in 2 places: select.ts
and multiselect.ts
, the key difference is that the default has 4 spaces while the explicit usages have 2 spaces. This is likely what you noticed in the snapshots, the rendering would be different between the def and explicit values. The param is used to show an elipsis when there are more options than can be displayed in the terminal window, indicating that there are more items above or below the visible options.
I would suggest to keep the param but standardize the spacing. The explicit usages in both select.ts
and multi-select.ts
use 2 spaces which seems to be the intended design. We could update the default in limit-options.ts
to match this standard of 2 spaces instead of 4. This way, we maintain the flexibility of the param (in case someone wants to customize it) while having consistent behavior.
Let me know if you want me to move forward with this recommendation 😁
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the multi-select prompt had no spaces before, and now has two, because of this
that's what i want to understand. does it look better with the two spaces? why were they added?
looks good to me! feel free to merge |
hey @grabbou can you open an issue please? 😁 |
Related: #203 💣