Skip to content
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

create-svelte: Add descriptions to select options #5221

Merged
merged 10 commits into from
Jun 22, 2022

Conversation

gtm-nayan
Copy link
Contributor

@gtm-nayan gtm-nayan commented Jun 18, 2022

closes #5217

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

  • If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpm changeset and following the prompts. All changesets should be patch until SvelteKit 1.0

Here's how the descriptions look
image

@changeset-bot
Copy link

changeset-bot bot commented Jun 18, 2022

🦋 Changeset detected

Latest commit: f3ae911

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

This PR includes changesets to release 1 package
Name Type
create-svelte 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

value: 'checkjs'
},
{
title: 'With TypeScript annotations',
Copy link
Member

Choose a reason for hiding this comment

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

I believe it's not just annotations, but the actual source is written in TypeScript

Suggested change
title: 'With TypeScript annotations',
title: 'Source is written TypeScript',

{ title: 'Type-checked JavaScript', value: 'checkjs' },
{ title: 'TypeScript', value: 'typescript' },
{
title: 'With JavaScript comments (JSDoc)',
Copy link
Member

Choose a reason for hiding this comment

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

might need a bit more explanation of what this is. but hopefully this isn't too long

Suggested change
title: 'With JavaScript comments (JSDoc)',
title: 'Runs TypeScript's type checking against JavaScript comments (JSDoc)',

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Guess it could be put into the descriptions, how's this?

image
image

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be better if it would be at same line... now it looks like another option.

Copy link
Member

Choose a reason for hiding this comment

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

I agree it looks nicer on the same line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's coming from how the prompts library handles descriptions, if they're too long to fit on one line they start from the next, so it has to be put in the title

image

Copy link
Contributor

Choose a reason for hiding this comment

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

Type-Checked JavaScript - check for types in JS with JSDoc.
TypeScript - check for types with TS.
None - Don't check for types.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or how it was in #5221 (comment) but shortcut JS and TS, and maybe replace last part with and run TSC to check types.

Copy link
Contributor Author

@gtm-nayan gtm-nayan Jun 21, 2022

Choose a reason for hiding this comment

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

Turns out we can cheat a bit with kleur to make parts of the title look like the description to avoid the auto-wrap behavior.

image

Ended up with this, which I think looks nice and readable.

@benmccann benmccann added the feature / enhancement New feature or request label Jun 21, 2022
@Rich-Harris
Copy link
Member

How about this?

image

@dummdidumm
Copy link
Member

Another one:

Do you want type checking?

  • Yes, using JavaScript with JSDoc comments
  • Yes, using TypeScript
  • No

@Rich-Harris
Copy link
Member

The reason I proposed

Use TypeScript?
> Yes, using JSDoc comments
  Yes, using TypeScript syntax
  No

is because 'JSDoc comments' by itself is too vague. We're specifically using the TypeScript flavour/subset of JSDoc (allowing us to e.g. import .d.ts files, which I don't believe is something JSDoc understands by itself), and we're configuring TypeScript with a jsconfig.json file. I don't think we can do an adequate job of communicating what this option actually does without putting the word 'TypeScript' front and center.

@gtm-nayan
Copy link
Contributor Author

"Use TypeScript?" by itself would likely be interpreted as the TypeScript language itself.

Add TypeScript's type checking?
> Yes, using JavaScript with JSDoc comments
  Yes, using TypeScript syntax
  No

@Mlocik97
Copy link
Contributor

Use TSC (TypeScript Compiler) to check for types?
> Yes, using JavaScript with JSDoc comments
  Yes, using TypeScript
  No

another option.

@Rich-Harris
Copy link
Member

I tweaked @gtm-nayan's suggestion in #5221 (comment) very slightly and committed it, so that the diff is once again legible. I think we're in a good place so I'll approve the PR, but will leave it open in case we want to bikeshed further

@Rich-Harris Rich-Harris merged commit d745435 into sveltejs:master Jun 22, 2022
@gtm-nayan gtm-nayan deleted the create-svelte-descriptions branch June 27, 2022 12:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature / enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add description to options when initializing new project
7 participants