Skip to content
This repository has been archived by the owner on Sep 6, 2018. It is now read-only.

Fonts refactor to use constants #58

Merged
merged 3 commits into from
Jul 7, 2017

Conversation

djbe
Copy link
Member

@djbe djbe commented Jun 6, 2017

Refactored the generated code to use static let constants instead of cases, as there is no real use case to want to enumerate over a font name.

There were also some issues with the font registration mechanism, which was broken if:

  • the font file had a different name than the font name
  • a different extension than "ttf" or "otf"

The fonts were also always re-registered if the font name was different from the font family name. We now directly use the path we receive from the parser, and match using the font family name from the parser.

@djbe
Copy link
Member Author

djbe commented Jun 6, 2017

Note that the branch name has "breaking" in it, but the refactored code turns out to be compatible, so yay! 🎉

@djbe djbe mentioned this pull request Jun 6, 2017
23 tasks
@djbe djbe force-pushed the feature/breaking/fonts-refactor branch from 734244a to 45d7aba Compare June 8, 2017 02:02
@Liquidsoul
Copy link
Member

I may see one use case where you would want to iterate over the fonts:

  • register the fonts ahead of time (e.g. at App startup)

I've done this by adding a static let all: [TheFont] in the enums and then do a all.forEach().register().

@@ -27,10 +27,10 @@ You can customize some elements of this template by overriding the following par
```swift
enum FontFamily {
enum SFNSDisplay: String, FontConvertible {
Copy link
Member

Choose a reason for hiding this comment

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

The String, FontConvertible conformance has been removed in your stencils.

@Liquidsoul
Copy link
Member

I've made a gist with a enum version with your fixes and the all option on each family name.

@djbe
Copy link
Member Author

djbe commented Jun 14, 2017

Hmm, my original point still stands, there's no reason to enumerate over the fonts, because you can't convert a string to the correct font (using rawValue) unless you already know which font family you're targeting.

That doesn't mean we can't add a all static constant for those that might want it.

@Liquidsoul
Copy link
Member

Ok that's fine by me. We just need to remove the : String, FontConvertible part in the markdown files then 😄

@AliSoftware
Copy link
Collaborator

@Liquidsoul does my last commit fix your questions? If so could you approve the PR so we can merge? Thx 😉

@AliSoftware AliSoftware force-pushed the feature/breaking/fonts-refactor branch from 97777a7 to ff2eddf Compare July 7, 2017 18:35
@Liquidsoul
Copy link
Member

@AliSoftware LGTM 👍

@AliSoftware AliSoftware merged commit 2389ac1 into master Jul 7, 2017
@AliSoftware AliSoftware deleted the feature/breaking/fonts-refactor branch July 7, 2017 21:35
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants