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

Ansi link #7751

Merged
merged 11 commits into from
Jan 15, 2023
Merged

Ansi link #7751

merged 11 commits into from
Jan 15, 2023

Conversation

NotLebedev
Copy link
Contributor

@NotLebedev NotLebedev commented Jan 14, 2023

Description

Add ansi link command that adds special ansi escape sequence to create a link in terminal:

image

This command accepts two arguments: uri of link and optional link text. If second parameter is not present link is displayed as is:

image

More info on links in terminals https://gist.github.com/egmontkob/eb114294efbcd5adb1944c9f3cb5feda

User-Facing Changes

New ansi link command

Tests + Formatting

Don't forget to add tests that cover your changes.

Make sure you've run and fixed any issues with these commands:

  • cargo fmt --all -- --check to check standard code formatting (cargo fmt --all applies these changes)
  • cargo clippy --workspace -- -D warnings -D clippy::unwrap_used -A clippy::needless_collect to check that you're using the standard code style
  • cargo test --workspace to check that all tests pass

After Submitting

If your PR had any user-facing changes, update the documentation after the PR is merged, if necessary. This will help us keep the docs up to date.

@fdncred
Copy link
Collaborator

fdncred commented Jan 14, 2023

Just a question. I'm wondering if it's more nushelly to make this 'https://nushell.sh' | ansi link --title nushell-website?

@NotLebedev
Copy link
Contributor Author

Changed it to work the same way as ansi gradient. Now it accepts strings, lists, and tables and records with cell path

image

(Type::Record(vec![]), Type::Record(vec![])),
])
.named(
"text",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
"text",
"title",

It just seems to me that --title is the better name for this parameter than --text. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so. For example in HTML title attribute has meaning of tooltip. For example:

<a href="https://nushell.sh" title="A new type of shell">Nushell</a>

generates

image

I think naming parameter titile may be confusing

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's the point I'm making. In your html example, the title is "A new type of shell" so the link shows "A new type of shell" instead of showing "https://nushell.sh". That's why I'm suggesting it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly. But the --text flag controls text of link, not tooltip. This is a call equivalent to my html example:

image

And as far as i understand there is no way to alter tooltip. In my screenshot it is generated by windows terminal and just shows what content of link is hidden

image

Copy link
Collaborator

Choose a reason for hiding this comment

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

I know of no way to control the tooltip either, but I wasn't talking about the tool tip. I prefer title but I'm not going to die on this hill. It's not that big of a deal. I'll let someone else accept or deny this PR.

@fdncred
Copy link
Collaborator

fdncred commented Jan 14, 2023

I really like this and thanks for making those changes. It definitely feels more like nushell now.

@kubouch kubouch merged commit a909c60 into nushell:main Jan 15, 2023
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