-
Notifications
You must be signed in to change notification settings - Fork 28
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
example improvements #184
base: main
Are you sure you want to change the base?
example improvements #184
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And here I thought Termion was low-dependency...
Thanks for the changes! I've made a few comments.
@@ -41,6 +41,7 @@ path = "examples/html2text.rs" | |||
env_logger = "0.10.1" | |||
argparse = "0.2.2" | |||
log = "0.4.20" | |||
yansi = { version = "1.0.1", features = ["hyperlink"] } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should remove the termion dependency if it's no longer used.
Image(_) => { | ||
styled = match annotation { | ||
Default => styled, | ||
Link(url) => styled.link(url).blue().underline().to_string(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the hyperlinks should be on by default, but fine with an opt in option (--hyperlinks
or similar).
start.push(format!("{}", Fg(Blue))); | ||
finish.push(format!("{}", Fg(Reset))); | ||
format!( | ||
"{} {}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can't add extra visible text here - the colour map function is not expected to change the size of any text, or it would mess up the formatting. As above, the hyperlink should only be done with an option to enable it.
} | ||
} | ||
Strikeout => { | ||
if !have_explicit_colour { | ||
start.push(format!("{}", Fg(LightBlack))); | ||
finish.push(format!("{}", Fg(Reset))); | ||
styled.strike().to_string() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The text should already be struck out already by this point (using U+336).
yansi
instead oftermion
for colouring