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

Draw Glyphs #179

Merged
merged 25 commits into from
Feb 10, 2022
Merged

Draw Glyphs #179

merged 25 commits into from
Feb 10, 2022

Conversation

wcandillon
Copy link
Contributor

@wcandillon wcandillon commented Feb 3, 2022

cc @christikaes

  • Write documentation
  • Rename Font to IFont for consistency
  • Rename <Text value=" to <Text text=" for consistency
  • Implement font methods

@wcandillon wcandillon requested a review from chrfalch February 3, 2022 09:20
@wcandillon wcandillon self-assigned this Feb 3, 2022
@wcandillon wcandillon changed the title Graw Glyphs Draw Glyphs Feb 3, 2022
@@ -9,7 +9,7 @@ const { width, height } = Dimensions.get("window");
export const COLS = 8;
export const ROWS = 15;
export const SYMBOL = { width: width / COLS, height: height / ROWS };
const symbols = "abcdefghijklmnopqrstuvwxyz".split("");
const symbols = "abcdefghijklmnopqrstuvwxyz".toUpperCase().split("");
Copy link
Contributor

Choose a reason for hiding this comment

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

It will be amazing to have the entire Latin extended characters just for reference, what do you think?

Both lowercase and uppercase:
ĀāĂ㥹ĆćĈĉĊċČčĎďĐđĒēĔĕĖėĘęĚěĜĝĞğĠġĢģĤĥĦħĨĩĪīĬĭĮįİıIJijĴĵĶķĸĹĺĻļĽľĿŀŁłŃńŅņŇňŊŋŌōŎŏŐőŒœŔŕŖŗŘřŚśŜŝŞşŠšŢţŤťŦŧŨũŪūŬŭŮůŰűŲųŴŵŶŷŸŹźŻżŽž
Only lowercase:
āăąćĉċčďđēĕėęěĝğġģĥħĩīĭįi̇ıijĵķĸĺļľŀłńņňŋōŏőœŕŗřśŝşšţťŧũūŭůűųŵŷÿźżž

Source

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed this needs to be tested. In this particular example, we use a custom font that only supports these codepoints.

@wcandillon wcandillon marked this pull request as ready for review February 3, 2022 13:05
Copy link

@christikaes christikaes left a comment

Choose a reason for hiding this comment

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

Yay! It's happening!!! 🎉 🎉

I think the API looks correct but the examples use codepoints instead of GlyphIds. I left a few comments where I saw that being used - let me know if that doesn't make sense!

docs/docs/text/glyphs.md Outdated Show resolved Hide resolved
package/src/skia/Canvas.ts Show resolved Hide resolved
example/src/Examples/Matrix/Symbol.tsx Outdated Show resolved Hide resolved
Copy link

@christikaes christikaes left a comment

Choose a reason for hiding this comment

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

Looks great! I personally likely won't use getGlyphIds because we already have functions for that when we do our text shaping calculations but I can see how it's useful in these examples 👍 Thanks for updating all of this to use glyphIds instead of codepoints! 🎉 I just noticed one small typo in the example here:

@@ -29,34 +28,10 @@ export const HelloWorld = () => {
y={0}
value="Hello World"
familyName="serif"
size={32}
text={32}

Choose a reason for hiding this comment

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

Did you mean to update value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes thanks 🙌🏼

Copy link

@christikaes christikaes left a comment

Choose a reason for hiding this comment

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

Looks great!

Copy link
Contributor

@chrfalch chrfalch left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@wcandillon wcandillon merged commit e2f8cbe into main Feb 10, 2022
@wcandillon wcandillon deleted the feat/drawGlyphs branch February 28, 2022 12:34
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.

5 participants