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

feat(components): move font styling out of Style and use families #879

Merged
merged 18 commits into from Jun 3, 2020

Conversation

zbaylin
Copy link
Member

@zbaylin zbaylin commented Jun 2, 2020

This creates a couple API changes:

  • the introduction of a FontFamily with a t that can be made with system and fromFile
  • the introduction of FontProps with family, weight, italicized, and monospaced information
  • the migration to specifying these FontProps directly on the Text component/node

@zbaylin zbaylin requested a review from bryphe June 2, 2020 20:30
src/Font/FontFamily.rei Outdated Show resolved Hide resolved
src/Font/FontFamily.rei Outdated Show resolved Hide resolved
let fromFile: (~variant: variantSolver=?, string) => t;
let system: string => t;

let default: t;
Copy link
Member

Choose a reason for hiding this comment

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

Nice! It's great to finally have a default!

let weight = _ => 1;
};

module FontFamilyCache = Lru.M.Make(FontFamilyHashable, FontDescriptorWeight);
Copy link
Member

Choose a reason for hiding this comment

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

Good idea to use the LruCache, to help bound the number of familes we keep in memory!

)
};

module FontFamilyHashable = {
Copy link
Member

Choose a reason for hiding this comment

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

Nice 👍 It's great that these implementation details aren't exposed to consumers of the FontFamily module - this is all tucked away behind the interface.

Comment on lines +18 to +21
val mutable _fontFamily = Family.default;
val mutable _fontWeight = Weight.Normal;
val mutable _italicized = false;
val mutable _monospaced = false;
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for adding these!

src/Font/FontFamily.re Outdated Show resolved Hide resolved
src/UI/FontProps.re Outdated Show resolved Hide resolved
src/Font/FontFamily.re Outdated Show resolved Hide resolved
@zbaylin zbaylin requested a review from bryphe June 2, 2020 23:23
Comment on lines +168 to +173
pub setFontFamily = fontFamily => {
if (_fontFamily !== fontFamily) {
_this#markLayoutDirty();
};
_fontFamily = fontFamily;
};
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for all the work to wire this up!

*. Text.getLineHeight(
~fontFamily=
Family.toPath(
_fontFamily,
Copy link
Member

Choose a reason for hiding this comment

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

One thing to think about is, maybe later, we could have the Family return a Font.t directly. Not a blocker for this change, though.

src/UI/TextNode.re Outdated Show resolved Hide resolved
Copy link
Member

@bryphe bryphe left a comment

Choose a reason for hiding this comment

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

Can't wait to have a real concept of font families!

@zbaylin zbaylin marked this pull request as ready for review June 3, 2020 00:25
@github-actions
Copy link

github-actions bot commented Jun 3, 2020

I have updated your lock dirs and formatted the code.
Please @zbaylin pull the last commit before pushing any more changes.

@zbaylin zbaylin merged commit 4cc62a5 into master Jun 3, 2020
Comment on lines +34 to +38
let equal = (a, b) =>
String.equal(a.familyName, b.familyName)
&& a.weight == b.weight
&& a.italicized == b.italicized
&& a.monospaced == b.monospaced;
Copy link
Member

Choose a reason for hiding this comment

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

Just out of curiosity, why not just a == b? Does String.equal differ in behaviour from ==?

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.

None yet

3 participants