Skip to content

Allow initializing Font with NSFont or UIFont#109

Closed
terhechte wants to merge 1 commit intoJohnSundell:masterfrom
terhechte:patch-1
Closed

Allow initializing Font with NSFont or UIFont#109
terhechte wants to merge 1 commit intoJohnSundell:masterfrom
terhechte:patch-1

Conversation

@terhechte
Copy link

This would allow using a custom monospace font for highlighting. Which is especially useful if, say, the code is embedded into some sort of presentation 😊. An assert could be added to make sure the font is monospace, but I feel like that's not required. If somebody wants to display their code in Comic Sans, all the more power to them

}

/// Initialize an instance with an existing, custom, font.
public init(font: Loaded) {
Copy link
Owner

Choose a reason for hiding this comment

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

I think here we can drop the external parameter label, so rather than:

Font(font: nativeFont)

It would be:

Font(nativeFont)

Or what do you think @terhechte?

Copy link
Owner

Choose a reason for hiding this comment

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

Or, an alternative design would be to use something inspired by RawRepresentable, and name the external parameter label rawFont, or nativeFont.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, I like it. Makes it clearer. If you're fine with this change, I'd check out the repo and create a proper PR, seems CI failed (I did the editing right on Github, so I never tried to compile it ;)

Copy link
Owner

Choose a reason for hiding this comment

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

Sounds great! 👍

@terhechte
Copy link
Author

Closed in favor of #111

@terhechte terhechte closed this Jun 7, 2020
@terhechte terhechte deleted the patch-1 branch June 7, 2020 07:47
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.

2 participants