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

ui: add color to help text #5716

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

gechr
Copy link
Contributor

@gechr gechr commented Feb 15, 2025

I appreciate that this might not be to everyone's taste, but I find help text output more readable with colours.

I chose the default clap v3 colours since that's what I've become used to with Rust tools, but happy to tweak as desired, or close this PR entirely if the plain appearance is the jj maintainers' preference.

Before

image

After

image

Checklist

If applicable:

  • I have updated CHANGELOG.md
  • I have updated the documentation (README.md, docs/, demos/)
  • I have updated the config schema (cli/src/config-schema.json)
  • I have added tests to cover my changes

@gechr gechr force-pushed the gc/qnzuxxrxpzoz branch 5 times, most recently from ce534cb to b09ca8a Compare February 15, 2025 22:41
@martinvonz
Copy link
Member

I personally like the colored styled. Can you remind me why clap4 switched away from it?

@gechr
Copy link
Contributor Author

gechr commented Feb 16, 2025

@martinvonz There's no strong justification as far as I can tell beyond "polishing up" - clap-rs/clap#4132

@gechr gechr force-pushed the gc/qnzuxxrxpzoz branch 2 times, most recently from 819f7ab to f75d8cf Compare February 16, 2025 02:14
@martinvonz
Copy link
Member

Thanks. I think that's quite well justified, actually. For reference, here's what it says, with some comments from me interleaved:

The greens and yellows are gone, instead using bold, underline, and dimmed: PR 4117 though some more work might be needed to enable colored help, depending on the needs

We use a lot of colors too and some people hate it. So I don't think this argument applies to jj. (We may want to define some kinds of "color scheme" or way of resetting colors without disabling them, but that's mostly unrelated to this PR.)

  • Color makes it easier to scan

+1

These are a bit relevant to us. I don't think the default clap colors conflict much with our color scheme. I don't think users are going to interpret the green commands as good and the yellow headings as warnings, for example. It seems reasonable for us to let the user define the colors in [colors] (see https://jj-vcs.github.io/jj/latest/config/#custom-colors-and-styles), however. I don't think that needs to be part of this PR.

So this PR looks good to me, but I'm not going to approve it yet because I think we should give others a chance to comment first.

Copy link
Contributor

@yuja yuja left a comment

Choose a reason for hiding this comment

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

I don't have strong feeling against the clap's default nor this PR.

cli/src/ui.rs Outdated Show resolved Hide resolved
@gechr gechr force-pushed the gc/qnzuxxrxpzoz branch 2 times, most recently from 04b381d to ee098aa Compare February 16, 2025 12:08
@gechr gechr enabled auto-merge February 18, 2025 00:32
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.

3 participants