workers-sdk icon indicating copy to clipboard operation
workers-sdk copied to clipboard

[Wrangler] Update the commands list

Open lrapoport-cf opened this issue 2 years ago • 13 comments

What this PR solves / how to test: This PR refreshes wrangler's default commands list, by removing old commands, updating visuals within the list, thus making it more consistent and easier to browse.

Before Screenshot 2024-02-26 at 18 50 41

After Screenshot 2024-02-26 at 18 50 01

Associated docs issue(s)/PR(s): https://github.com/cloudflare/cloudflare-docs/pull/11514

Author has included the following, where applicable:

Reviewer is to perform the following, as applicable:

  • Checked for inclusion of relevant tests
  • Checked for inclusion of a relevant changeset
  • Checked for creation of associated docs updates
  • Manually pulled down the changes and spot-tested

Note for PR author:

We want to celebrate and highlight awesome PR review! If you think this PR received a particularly high-caliber review, please assign it the label highlight pr review so future reviewers can take inspiration and learn from it.

lrapoport-cf avatar Aug 10 '23 21:08 lrapoport-cf

🦋 Changeset detected

Latest commit: a61500f162a98a004aeab68e7bcb2fbc17c5dd8b

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

This PR includes changesets to release 2 packages
Name Type
wrangler Patch
@cloudflare/vitest-pool-workers 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

changeset-bot[bot] avatar Aug 10 '23 21:08 changeset-bot[bot]

A wrangler prerelease is available for testing. You can install this latest build in your project with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9601015901/npm-package-wrangler-3735

You can reference the automatically updated head of this PR with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/3735/npm-package-wrangler-3735

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9601015901/npm-package-wrangler-3735 dev path/to/script.js
Additional artifacts:
npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9601015901/npm-package-create-cloudflare-3735 --no-auto-update
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9601015901/npm-package-cloudflare-kv-asset-handler-3735
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9601015901/npm-package-miniflare-3735
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9601015901/npm-package-cloudflare-pages-shared-3735
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9601015901/npm-package-cloudflare-vitest-pool-workers-3735

Note that these links will no longer work once the GitHub Actions artifact expires.


[email protected] includes the following runtime dependencies:

Package Constraint Resolved
miniflare workspace:* 3.20240610.1
workerd 1.20240610.1 1.20240610.1
workerd --version 1.20240610.1 2024-06-10

Please ensure constraints are pinned, and miniflare/workerd minor versions match.

github-actions[bot] avatar Aug 11 '23 20:08 github-actions[bot]

Codecov Report

Attention: Patch coverage is 98.30508% with 1 line in your changes missing coverage. Please review.

Project coverage is 75.44%. Comparing base (2789f26) to head (00c640f). Report is 296 commits behind head on main.

:exclamation: Current head 00c640f differs from pull request most recent head 8bd2bc7

Please upload reports for the commit 8bd2bc7 to get more accurate results.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3735      +/-   ##
==========================================
+ Coverage   72.44%   75.44%   +2.99%     
==========================================
  Files         331      223     -108     
  Lines       17298    12332    -4966     
  Branches     4422     3188    -1234     
==========================================
- Hits        12532     9304    -3228     
+ Misses       4766     3028    -1738     
Files Coverage Δ
packages/wrangler/src/ai/index.ts 100.00% <ø> (ø)
packages/wrangler/src/constellation/index.ts 100.00% <100.00%> (ø)
packages/wrangler/src/hyperdrive/index.ts 100.00% <ø> (ø)
packages/wrangler/src/index.ts 83.72% <100.00%> (-6.59%) :arrow_down:
packages/wrangler/src/kv/index.ts 100.00% <ø> (ø)
packages/wrangler/src/pages/index.ts 84.00% <100.00%> (-2.21%) :arrow_down:
...wrangler/src/queues/cli/commands/consumer/index.ts 100.00% <ø> (+15.38%) :arrow_up:
packages/wrangler/src/queues/cli/commands/index.ts 100.00% <100.00%> (ø)
packages/wrangler/src/secret/index.ts 88.51% <ø> (+5.18%) :arrow_up:
packages/wrangler/src/vectorize/index.ts 100.00% <ø> (ø)
... and 1 more

... and 237 files with indirect coverage changes

codecov[bot] avatar Oct 24 '23 15:10 codecov[bot]

Great effort combing through all of this!

A couple subjective points:

  • I think the emojis should have a space after them before the text begins
  • I think using the same emoji for all commands reduces their "fun" quotient (I bet I sound fun right now 👀) and I don't think the consistency there is of any benefit
  • I like the introduction of a consistent emoji for all experimental commands – maybe we could use a different one that's less subtle?

RamIdeas avatar Nov 03 '23 17:11 RamIdeas

@RamIdeas thanks for the feedback! the emoji changes are based on a proposal from the design team -- i'll get them looped into the discussion. for my two cents, i agree that the consistency has a lower fun quotient 😄, but i do think it adds some informational value that the more assorted collection lacks: it signals that there is a loose relationship between the items in the different emoji groups, and also distracts less from the command descriptions.

lrapoport-cf avatar Nov 03 '23 17:11 lrapoport-cf

cc @Meahaa

lrapoport-cf avatar Nov 03 '23 17:11 lrapoport-cf

Excellent points let's:

  1. Add the space
  2. Keep the emoji's agree it's more fun to have them and we can take a pass in the future to make sure they actually make sense
  3. I'm not following too clearly here - I liked the orange text because it's a nice brand call out

Meahaa avatar Nov 03 '23 17:11 Meahaa

  1. I'm not following too clearly here - I liked the orange text because it's a nice brand call out

I think you're referring to the "open beta" text in the figma design doc which I think being on-brand orange is great!

I was referring to the ⚑ flag emoji which blends into the text a little – a non-exhaustive set of alternatives to consider:

  • 🧪 test tube
  • 👩‍🔬 scientist
  • 🥼 lab coat
  • 🧫 petri dish
  • 🔬 microscope

RamIdeas avatar Nov 03 '23 18:11 RamIdeas

@Meahaa - can you take a look at @RamIdeas' last comment? Should we update that last emoji? @lrapoport-cf - can we rebase and fix the conflicts while updating to fit the agreed points between @RamIdeas and @Meahaa ?

petebacondarwin avatar Feb 19 '24 10:02 petebacondarwin

ooh I like the test tube let's go with that!

Meahaa avatar Feb 20 '24 19:02 Meahaa

Excellent points let's:

  1. Add the space
  2. Keep the emoji's agree it's more fun to have them and we can take a pass in the future to make sure they actually make sense
  3. I'm not following too clearly here - I liked the orange text because it's a nice brand call out

@Meahaa I am taking over this PR to cross it over the finish line. Can you please clarify point 2 for me? Do we keep the current emojis as they are, or do we change them as per design?

CarmenPopoviciu avatar Feb 26 '24 14:02 CarmenPopoviciu

@Meahaa @RamIdeas @petebacondarwin I've added a before and after screenshot to the description of this PR. Before reviewing the code, is there consensus that, visually, things are as we want them to be? <3

CarmenPopoviciu avatar Feb 26 '24 18:02 CarmenPopoviciu

I read that @Meahaa wrote:

Keep the emoji's agree it's more fun to have them and we can take a pass in the future to make sure they actually make sense

So I think that means we are all agreed that we should not switch out the mix of emojis for the boring few?

petebacondarwin avatar Feb 26 '24 18:02 petebacondarwin

Yup! Let's just keep the current emojis for now! Agree with the fact that they make them more fun for now and we can always update to make them even better later!

Meahaa avatar Mar 06 '24 20:03 Meahaa