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

SImplify "astro add" by removing confusing multi-select #3715

Merged
merged 8 commits into from
Jun 27, 2022

Conversation

FredKSchott
Copy link
Member

@FredKSchott FredKSchott commented Jun 26, 2022

Changes

  • Our "astro add" multi-select had been causing users trouble.
  • This change solves the usability issues by just removing the multi-select flow.
  • The multi-select was a neat idea, but this new static output contains all the same information without the usability issues. Plus, it teaches you how to use the real CLI command.
  • The analogy that I have come to think of this as: "npm doesn't have a multi-select for 'npm install', and they do just fine, so why should we?"

Bonus! create-astro also updated with a few style changes and bug fixes.
Bonus! Added link to our full integration catalog.
Bonus! Cleaned up the printHelp() function to be a bit more pretty across all the commands, and updated its API to be a bit more flexible.

Screenshot: astro add multi-select replaced with this output

Screen Shot 2022-06-25 at 11 33 16 PM

Recording: The new, updated create-astro flow

Screen.Recording.2022-06-26.at.10.48.21.PM.mov

Testing

  • Tested manually (astro add isn't tested yet)
  • This mainly removes logic, so bug risk should be lower than normal

Docs

@changeset-bot
Copy link

changeset-bot bot commented Jun 26, 2022

🦋 Changeset detected

Latest commit: b04410c

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

This PR includes changesets to release 9 packages
Name Type
astro Patch
@e2e/astro-component Patch
@e2e/lit-component Patch
@e2e/preact-component Patch
@e2e/react-component Patch
@e2e/solid-component Patch
@e2e/svelte-component Patch
@e2e/e2e-tailwindcss Patch
@e2e/ts-resolution 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

@github-actions github-actions bot added the pkg: astro Related to the core `astro` package (scope) label Jun 26, 2022
@hippotastic
Copy link
Contributor

hippotastic commented Jun 26, 2022

Does the command support specifying multiple integrations to add in one call? If so, that would be great, and it should be reflected in the help output somehow.

So rather than astro add [integration] we could write something like astro add [integration names], [integrations...] or anything else that you see fit.

What I also currently find a bit misleading is that you're introducing astro add in the usage line as a command to install [integrations], but then provide two lists: One of UI Frameworks (huh?), and one of Integrations. New developers could be confused if UI Frameworks are something else than integrations.

I'd personally also be confused why those integrations are all "recommended". Does that mean I should install all of them?

I'd therefore propose to change the list headers to something like Example: Add UI Framework Support and Example: Add Other Popular Integrations.

@delucis
Copy link
Member

delucis commented Jun 26, 2022

Hippo's ideas seem smart — plus one on those.

Re: docs — yes we describe the current CLI output in detail in our install guide, which will need updating.

In fact, how will this work as part of create-astro? Do we just remove the add step and recommend running it when the setup CLI is done?

@github-actions github-actions bot added the pkg: create-astro Related to the `create-astro` package (scope) label Jun 27, 2022
@FredKSchott
Copy link
Member Author

Agreed! Updated copy based on @hippotastic's comments.

Re: docs — yes we describe the current CLI output in detail in our install guide, which will need updating.

Great! PR to update created: withastro/docs#867

In fact, how will this work as part of create-astro? Do we just remove the add step and recommend running it when the setup CLI is done?

Good call out! I updated the PR to update create-astro for this change. Video preview added to OP.

Copy link
Member

@delucis delucis left a comment

Choose a reason for hiding this comment

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

I’m a big plus one on these changes. The multi-select was also really hard to describe in docs, so this will make our life easier 😅

Spotted a couple of tiny things, but otherwise LGTM!

packages/astro/src/core/logger/node.ts Outdated Show resolved Hide resolved
await logAndWait('');
await logAndWait(`Stuck? Come join us at ${bold(cyan('https://astro.build/chat'))}`, 1000);
await logAndWait(dim('Good luck out there, astronaut.'));
await logAndWait('', 300);
Copy link
Member

Choose a reason for hiding this comment

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

Nice touch with the staggered output!

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! I was kind of just playing around at first but it feels a lot more human.

The 1000 wait before "good luck out there" is a bit of a cheesy touch, so if anyone has any thoughts I'm okay to scrap it or shrink it down to a smaller wait. You can see it in action in the video above.

Copy link
Member

Choose a reason for hiding this comment

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

I like it! I bet there are some speed freaks who’d be disgusted, but you need to develop patience for a journey to the stars.

packages/create-astro/test/install-step.test.js Outdated Show resolved Hide resolved
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 good! Had a few small comments and would love to see existing feedback addressed before approving.

.changeset/perfect-drinks-fold.md Show resolved Hide resolved
packages/create-astro/src/index.ts Show resolved Hide resolved
packages/create-astro/src/index.ts Show resolved Hide resolved
FredKSchott and others added 3 commits June 27, 2022 10:46
Co-authored-by: Chris Swithinbank <swithinbank@gmail.com>
Co-authored-by: Chris Swithinbank <swithinbank@gmail.com>
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.

LGTM once tests are passing!

@FredKSchott FredKSchott merged commit 4d6d864 into main Jun 27, 2022
@FredKSchott FredKSchott deleted the astro-add-update branch June 27, 2022 21:15
@astrobot-houston astrobot-houston mentioned this pull request Jun 27, 2022
@astrobot-houston astrobot-houston mentioned this pull request Jun 28, 2022
SiriousHunter pushed a commit to SiriousHunter/astro that referenced this pull request Feb 3, 2023
* wip

* update create-astro for new astro add

* update copy

* update git prompt

* Update packages/astro/src/core/logger/node.ts

Co-authored-by: Chris Swithinbank <swithinbank@gmail.com>

* Update packages/create-astro/test/install-step.test.js

Co-authored-by: Chris Swithinbank <swithinbank@gmail.com>

* update git prompt

* update test

Co-authored-by: Chris Swithinbank <swithinbank@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope) pkg: create-astro Related to the `create-astro` package (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants