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

Add VictorMono #364

Merged
merged 1 commit into from
Aug 11, 2019
Merged

Add VictorMono #364

merged 1 commit into from
Aug 11, 2019

Conversation

kule
Copy link
Contributor

@kule kule commented Jul 27, 2019

Description

Added the VictorMono font

Requirements / Checklist

  • [ x ] Read the Contributing Guidelines

  • [ x ] Read or at least glanced at the FAQ

  • [ x ] Read or at least glanced at the Wiki

  • [ x ] Scripts execute without error (if necessary):

    • If any of the scripts were modified they have been tested and execute without error, e.g.:
      • ./font-patcher Inconsolata.otf --fontawesome --octicons --pomicons
      • ./gotta-patch-em-all-font-patcher\!.sh Hermit
  • [ x ] Extended the README and documentation if necessary, e.g. You added a new font please update the table

@kule
Copy link
Contributor Author

kule commented Jul 27, 2019

Not sure how to interpret that ci error? If there’s something I need to tweak let me know...

@ryanoasis
Copy link
Owner

Thanks @kule

You can ignore the build error. That's from some shellcheck violations. To be fixed...

@mhartington
Copy link

Note that this has the same issue as listed here

#254

Because Victor ships with ligatures already, the font needs to either have a config to disable the fi/fl ligatures. I've attempted this myself, but to no success.

@ryanoasis
Copy link
Owner

Thanks @mhartington

There is another option that I had forgot about, that is using the careful flag. You would lose any glyphs that would stomp over but that would ensure ligatures don't get replaced.

Cross posting reply from #254:

Sorry for the late reply, yeah I think the real solution will involve not clobbering the ligatures but that will involve moving some of the codepoints of some glyphs. That would probably result in a 3.0 release (breaking change).

There is another workaround worth mentioning (if one patches themselves). that is using the --careful flag. That will ensure existing glyphs don't get clobbered, for example:

left: original font
center: straight up patch
right: patch with the --careful flag
VictorMono-Regular-patching-comparisons

Ref: #57 (comment)

@ryanoasis
Copy link
Owner

I think this looks good, at least good enough to be in master. There are issues such as the one mentioned that probably need to be reconciled before a release. That seems out of scope of this PR since other fonts with ligatures currently have the same issue 😄

Also looks like you @kule instructions perfectly so appreciate that

@ryanoasis ryanoasis merged commit 091ae08 into ryanoasis:master Aug 11, 2019
@ryanoasis ryanoasis added this to the v2.1.0 milestone Feb 2, 2020
@ryanoasis
Copy link
Owner

fixing milestone, need to update changelog and release and all-contributors

LNKLEO pushed a commit to LNKLEO/Nerd that referenced this pull request Nov 24, 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