Skip to content

Clarify CLI instructions#212

Merged
manuel3108 merged 6 commits intosveltejs:mainfrom
sacrosanctic:patch-1
Oct 24, 2024
Merged

Clarify CLI instructions#212
manuel3108 merged 6 commits intosveltejs:mainfrom
sacrosanctic:patch-1

Conversation

@sacrosanctic
Copy link
Copy Markdown
Contributor

@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented Oct 23, 2024

🦋 Changeset detected

Latest commit: b245c9b

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

This PR includes changesets to release 1 package
Name Type
sv Patch

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
Copy Markdown
Member

@AdrianGonz97 AdrianGonz97 left a comment

Choose a reason for hiding this comment

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

thanks!

@pkg-pr-new
Copy link
Copy Markdown

pkg-pr-new Bot commented Oct 23, 2024

Open in Stackblitz

pnpm add https://pkg.pr.new/sveltejs/cli/sv@212

commit: b245c9b

Comment thread packages/cli/commands/add/index.ts Outdated
@benmccann
Copy link
Copy Markdown
Member

Do we need the same hint if you run create, etc ?

@AdrianGonz97
Copy link
Copy Markdown
Member

this change is already shared with create since as it invokes runAddCommand

Copy link
Copy Markdown
Member

@benmccann benmccann left a comment

Choose a reason for hiding this comment

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

Oh, that's pretty weird. You'd want it to show up in the first create question - not the third

@AdrianGonz97
Copy link
Copy Markdown
Member

AdrianGonz97 commented Oct 23, 2024

it only shows up on the multi-select where there's a difference between space and enter. the others are just normal select prompts (only enter applies)

@manuel3108
Copy link
Copy Markdown
Member

I also understand it the way that the core problem is the multiselect.
But I think we should be consistent and also add it to the tailwind plugin multiselect:

image

@AdrianGonz97
Copy link
Copy Markdown
Member

AdrianGonz97 commented Oct 23, 2024

I thought about that and figured that it's fine to exclude it from succeeding* multi-select prompts as you've been "taught" what to do by the time you reach this prompt

@manuel3108
Copy link
Copy Markdown
Member

I thought about that and figured that it's fine to exclude it from succeeding* multi-select prompts as you've been "taught" what to do by the time you reach this prompt

Not necessarily, because you can just run npx sv add tailwindcss. But you could argue that if you know that command you also know how to navigate the multiselect.

@sacrosanctic
Copy link
Copy Markdown
Contributor Author

What is the reason for not adding hints to subsequent multi-selects? Is it because of clutter?

Isn't it always better to provide visual aid, I understand it's squares for multi-select and circles for single-select. But I find that hardly noticeable.

@benmccann
Copy link
Copy Markdown
Member

I'd have to see what it looks like, but my first reaction is that it'd be too noisy and we should mention it only once

@manuel3108
Copy link
Copy Markdown
Member

manuel3108 commented Oct 23, 2024

image

You can also try it yourself with

pnpx https://pkg.pr.new/sveltejs/cli/sv@407d856 create

I think this is totally fine and could be useful for some users.

@AdrianGonz97
Copy link
Copy Markdown
Member

seeing it now, I still think it adds unnecessary noise. this noisiness is also exacerbated when the instructions are kept on screen long after prompt submission

@sacrosanctic
Copy link
Copy Markdown
Contributor Author

Is it possible to add a check to only show the hint on the first multi-select the user encounters, or would that increase the code complexity.

If possible, this would reduce clutter and still have hints regardless of which branch the user takes.

@manuel3108
Copy link
Copy Markdown
Member

Let's only add this to the integration-selection question for now, as everyone seems to agree there. Let's get this merged and hear for user feedback

@manuel3108 manuel3108 merged commit aed1f42 into sveltejs:main Oct 24, 2024
@github-actions github-actions Bot mentioned this pull request Oct 22, 2024
@sacrosanctic sacrosanctic deleted the patch-1 branch October 24, 2024 18:51
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.

4 participants