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

refactor(cli): improve output aesthetics #6997

Merged
merged 7 commits into from
May 24, 2022
Merged

refactor(cli): improve output aesthetics #6997

merged 7 commits into from
May 24, 2022

Conversation

innocenzi
Copy link
Contributor

@innocenzi innocenzi commented Feb 20, 2022

Description

This PR is a minor improvement to the CLI output of the development server. While working on a plugin, I wanted to add some useful information to the terminal, and I thought the current output could be improved a bit.

Note: don't feel bad for straight up closing this PR - this is a small proposal and it would be totally fine if it was refused. Also I'm totally open on iterating over the design.

Output after this PR

Initial suggestion



Current output



What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the Commit Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@patak-dev
Copy link
Member

Nice! Good to try new things here. Some comments about the changes:

  • I like the new arrows. I think we could keep them.
  • I think that it is the right move to dim the "use host to expose" string, that info is not relevant to everyone. And now enough time has passed since we change the default. Probably it is too much dimmed though, I'm afraid we may have a11y issues. Maybe we could light it up a bit?
  • I like that the addresses are aligned now! Maybe we could also drop the : to make it cleaner?
  • dimming the starting time is interesting. I don't think we should do it yet. IIUC, the idea is to make it prominent to contrast with other bundlers. Maybe at one point, we could directly remove it? I vote to leave it as it was for now.
  • For the title, I prefer that it stays as it was. Maybe we should capitalize Vite though.

@bluwy
Copy link
Member

bluwy commented Feb 20, 2022

Perhaps another thing to consider is how this would look with plugins that add other CLI entries, e.g. vite-plugin-inspect and vite-plugin-qrcode. With this change they may look slightly out of place.

Design-wise I still prefer the old one, but mostly for nostalgia reasons 😄

@patak-dev
Copy link
Member

That is a good point @bluwy. Maybe we could merge this PR (with modifications), or other PRs when we are in the Vite 3.0 beta? I think it may help then to have a refreshed CLI look. And plugins and frameworks would also be able to better target the style for the major

@patak-dev patak-dev added this to the 3.0 milestone Feb 20, 2022
@patak-dev patak-dev added the p2-nice-to-have Not breaking anything but nice to have (priority) label Feb 20, 2022
@Shinigami92
Copy link
Member

Shinigami92 commented Feb 20, 2022

The background color of VITE v2.8.4 is to bright and laggs accessibility in contrast

Otherwise I like it, especially that it now is a bit (2 lines) smaller

@innocenzi
Copy link
Contributor Author

innocenzi commented Feb 20, 2022

Hey all, there's a lot of valid feedback, thanks!

 
 

@bluwy
Perhaps another thing to consider is how this would look with plugins that add other CLI entries

I had the intent to PR vite-plugin-inspect if this was merged to update the design - I didn't know about vite-plugin-qrcode though (pretty cool, after taking a look!). That was when I worked on such a feature that I had the idea for this PR actually! Here is an example:

On this note, I found out that the new line at the end of the default logging design denies a potentially interesting design for plugins, which would be to simply add a line after the default ones:

I'm thinking that Vite could offer a basic API to add plugin-specific logging like this, instead of having to hook into configureServer and use server.config.logger.info directly. Maybe I'll explore this in another PR, this is out of scope for this one.

 
 

@Shinigami92
The background color of VITE v2.8.4 is to bright and laggs accessibility in contrast

@patak-dev
Probably it is too much dimmed though, I'm afraid we may have a11y issues.

Good point!

I'm not sure how to handle this because I only use standard terminal escape codes, which don't render the same depending on your terminal theme. In this case, I used gray for the dimmed parts to make it consistent between VSC and WT, because VSC doesn't render a different color using the standard dim, while WT uses a different opacity.

1️⃣ Here is what it looks like with VSC's default theme:

2️⃣ Here is an alternative design which uses bold for non-dimmed parts instead:

 
 

@patak-dev
For the title, I prefer that it stays as it was. Maybe we should capitalize Vite though.

Honestly that was the hardest part of the design haha. I took inspiration from Vitest on this one. I like the contrast between the header part and the rest of the data.

3️⃣ Here is something that looks more like the current output:

4️⃣ Or with an offset:

 
 

@patak-dev
I like that the addresses are aligned now! Maybe we could also drop the : to make it cleaner?

5️⃣ Something like this?

 
 

Personal thoughts on your feedback (thanks again!):

  • My favorite design in this comment is 4️⃣.
  • I think it's okay to keep the dimmed parts gray because people with accessibility issues would most likely have themes with more contrasted colors.

@innocenzi
Copy link
Contributor Author

innocenzi commented Feb 20, 2022

For now I changed this PR to use design 4️⃣.

Example: vite-plugin-inspect and vite-plugin-laravel with this design:

@Shinigami92
Copy link
Member

From the screenshots, I also like 4️⃣ the most

Shinigami92
Shinigami92 previously approved these changes Feb 20, 2022
@patak-dev
Copy link
Member

patak-dev commented Feb 20, 2022

I think design 4 looks great (also 5 to me looks equally good)! And I like your idea of letting plugins like inspect provide other URLs to add to the list.

About using a background, I think it would look better if the font color will be black instead of white. I think Vitest is using that design as a play with the typical PASS, FAIL.

gray being ok for a11y because people may have their terminal theme adjusted could work... I don't know much about a11y in terminals to know, maybe we need to ask for help on this one.

IMO, Vite looks nicer than VITE (maybe it is just that I'm used to it). But the all caps version may work better after 3.0, because we are going to change the default dev port to 5173

VITE
5173

@innocenzi
Copy link
Contributor Author

@patak-dev
About using a background, I think it would look better if the font color will be black instead of white. I think Vitest is using that design as a play with the typical PASS, FAIL.

I personally have the feeling like white is clearer - but both gray and white don't pass WCAG AAA anyway :/

@patak-dev
IMO, Vite looks nicer than VITE

I am neutral about the casing of Vite vs VITE, so if you would like me to update the PR to title case instead of upper case, I can :)

(I didn't realize the new port was a leet version of Vite haha)

@patak-dev
Copy link
Member

It is kind of a secret, dont tell others. I vote for all caps 😁

@bholmesdev
Copy link
Contributor

Sorry to throw a wrench in this with another idea, but I'm really digging the |s we added to the new Astro CLI! Here's our output for reference. Also a big fan of using a block background for the framework name; adds some nice emphasis across light and dark themes.

Screen Shot 2022-03-10 at 1 10 15 PM

@Shinigami92 Shinigami92 self-requested a review May 18, 2022 14:56
Shinigami92
Shinigami92 previously approved these changes May 18, 2022
antfu
antfu previously approved these changes May 19, 2022
Copy link
Member

@antfu antfu left a comment

Choose a reason for hiding this comment

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

Love it! Also nice to have it as 3.0 so ppl could perceive more on the improvements 🫣

@innocenzi innocenzi dismissed stale reviews from antfu and Shinigami92 via b7f3360 May 19, 2022 16:57
@patak-dev
Copy link
Member

Love it! Also nice to have it as 3.0 so ppl could perceive more on the improvements 🫣

Totally! With this and the new docs, the impression of Vite will be different and that is good not only for the perceived progress. I think it is also good for users to easily identify if they are running v2 or v3 (or are reading the v2 or v3 docs)

)
urls.forEach(({ label, url: text }) => {
info(
` ${colors.green('➜')} ${colors.bold(label)}: ${' '.repeat(
Copy link
Contributor

Choose a reason for hiding this comment

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

Love the design. I prefer 2 spaces here like in No.3 design since I think the extra 2 spaces don't provide special meaning here.

Copy link
Member

Choose a reason for hiding this comment

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

I also prefer 2 spaces here. It is just a detail but aligning with the start of VITE feels nicer.

@patak-dev
Copy link
Member

We talked about the PR in today's team meeting, let's do this! ❤️
@innocenzi let's discuss the 2 vs 4 spaces proposal from @ydcjeff, and then we can merge it. If we have other ideas later, we can continue to iterate. @antfu mentioned today about an API for plugins to inject extra lines like vite-plugin-inspector. We can explore that in a future PR.

@innocenzi
Copy link
Contributor Author

Personally I prefer it indented since it creates a sort of block. I like the look of the indentation. That being said I'm totally fine with only two spaces.

Whichever you chose, I'm not home until Monday so I can't update the PR - anyone with write access to the target branch, feel free to update my PR :p

I also dig the idea to provide an API to inject lines to the output! And I agree it's out of scope there and should be discussed in another PR.

@patak-dev
Copy link
Member

I can update the PR to what we decide 👍🏼, don't worry. Thanks again for pushing for this change!

I see your point about creating a block. But to me with fewer spaces you also get a block because the arrows are small and your eyes focus on the start of Local and Network. I just tested and to me, 3 spaces could also work 👀

image

@innocenzi
Copy link
Contributor Author

Haha, I'm fine with either honestly. So if the majority prefers 2, let's do 2. I like 3 as well. :p

Maybe we can vote?

  • 2 spaces, 👍
  • 3 spaces, 🚀
  • 4 spaces, 👀

@bholmesdev
Copy link
Contributor

Haha, I'm fine with either honestly. So if the majority prefers 2, let's do 2. I like 3 as well. :p

Maybe we can vote?

  • 2 spaces, 👍
  • 3 spaces, 🚀
  • 4 spaces, 👀

I'll take the "I'm feeling lucky, let's ship it" option 🎲

Copy link
Member

@patak-dev patak-dev left a comment

Choose a reason for hiding this comment

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

Looks like 2 spaces won 👀
At least until we have an API for plugins to use to inject their own lines, I think it isn't a bad idea to use 2 so we don't need to update them right away
We could review again once we expose that API

Final state of the CLI with this PR
Screenshot 2022-05-24 at 17 56 12

@patak-dev patak-dev merged commit 809ab47 into vitejs:main May 24, 2022
@innocenzi innocenzi deleted the refactor/cli-output branch May 24, 2022 17:45
@innocenzi
Copy link
Contributor Author

Nice. Thanks for merging!

@patak-dev
Copy link
Member

Thank you for pushing this to the finish line @innocenzi 🙌🏼
It will make a big diff once we launch v3!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2-nice-to-have Not breaking anything but nice to have (priority)
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

7 participants