Skip to content

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

Merged
merged 15 commits into from
May 6, 2025
Merged

feat(core + prompts): adds autocomplete #288

merged 15 commits into from
May 6, 2025

Conversation

dreyfus92
Copy link
Member

@dreyfus92 dreyfus92 commented Apr 14, 2025

Related: #203 💣

Copy link

changeset-bot bot commented Apr 14, 2025

🦋 Changeset detected

Latest commit: 984a92d

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@clack/prompts Minor
@clack/core Minor

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

Copy link

pkg-pr-new bot commented Apr 14, 2025

@example/basic@example/changesets

npm i https://pkg.pr.new/bombshell-dev/clack/@clack/core@288
npm i https://pkg.pr.new/bombshell-dev/clack/@clack/prompts@288

commit: 984a92d

Copy link
Contributor

@MacFJA MacFJA left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO the hint always visible feel a bit overwhelming, that add a lot of gray (and informative) text
There is also some consistency issue

image

But I really like the presentation:

  • informative line above and under the content
  • the content being indented to separate it from the informative part

Copy link
Member

@natemoo-re natemoo-re left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks really awesome! A few minor points on the visual feedback, also noticing a bug with an empty line at the top and some issues switching from multiselect to filtering.

Screenshot 2025-04-18 at 8 39 13 AM

@dreyfus92 dreyfus92 requested a review from natemoo-re April 19, 2025 01:52
@dreyfus92 dreyfus92 changed the title feat: adds autocomplete feat(core + prompts): adds autocomplete Apr 19, 2025
@dreyfus92 dreyfus92 requested a review from MacFJA April 19, 2025 03:40
Copy link
Contributor

@MacFJA MacFJA left a 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

@MacFJA
Copy link
Contributor

MacFJA commented Apr 21, 2025

This PR will replace #124

@dreyfus92 dreyfus92 requested a review from 43081j April 28, 2025 16:35
@dreyfus92 dreyfus92 requested review from 43081j and MacFJA April 29, 2025 01:32
@dreyfus92 dreyfus92 requested a review from 43081j April 29, 2025 17:25
}

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(' ...');
Copy link
Collaborator

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

Copy link
Member Author

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.

Copy link
Collaborator

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

Copy link
Collaborator

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

Copy link
Member Author

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?

Copy link
Member Author

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 😁

Copy link
Collaborator

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?

@43081j
Copy link
Collaborator

43081j commented May 6, 2025

looks good to me!

feel free to merge

@dreyfus92 dreyfus92 merged commit f2c2b89 into main May 6, 2025
6 checks passed
@dreyfus92 dreyfus92 deleted the feat/autocomplete branch May 6, 2025 18:09
@dreyfus92 dreyfus92 mentioned this pull request May 6, 2025
12 tasks
@grabbou
Copy link

grabbou commented May 12, 2025

CleanShot 2025-05-13 at 00 55 06@2x

Feature wise this works awesome! Unfortunately, I am seeing this weird glitch where things appear multiple times. Using both Ghostty and built-in Mac terminal.

@dreyfus92
Copy link
Member Author

hey @grabbou can you open an issue please? 😁

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants