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

Fonts: DEMOS #1216

Closed
wants to merge 1 commit into from
Closed

Fonts: DEMOS #1216

wants to merge 1 commit into from

Conversation

ronyeh
Copy link
Collaborator

@ronyeh ronyeh commented Nov 5, 2021

This PR contributes examples in the vexflow/demos/fonts folder. It is part of the Font PR that has been extracted for easier code review & merging.

Since these files are independent from the core code, this PR is safe to merge. However, the demos won't actually work until the Font related improvements are merged (in the next couple of PRs).

// await Vex.Flow.setMusicFont('Bravura')
// await Vex.Flow.setMusicFont('Gonville')
// await Vex.Flow.setMusicFont('Petaluma')
await Vex.Flow.setMusicFont(fontName);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For vexflow-core.js, Flow.setMusicFont(fontName) is async. So you'll have to use await to make sure the font is loaded before proceeding to render the score.

<html>
<body>
<div id="output"></div>
<script src="../../build/vexflow-core-with-bravura.js"></script>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These are new! Simon of OSMD requested support for smaller builds without undesired fonts.

vexflow-core.js bundles zero fonts. Load fonts at runtime by calling Flow.setMusicFont(...fontNames).

vexflow-core-with-bravura.js bundles only the Bravura font by default. Other fonts can be dynamically loaded with a call to Flow.setMusicFont(...fontNames).

@@ -0,0 +1,46 @@
// node fonts.js > output.html
//
// This demo shows how to use node-canvas to load a font (woff / otf).
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

When run on the command line via node, the ChordSymbol test cases fall back to Times and Arial, because the web fonts are not available. This demo shows how we can register custom fonts like Roboto Slab and PetalumaScript so that they will appear in node-canvas.

@ronyeh ronyeh mentioned this pull request Nov 5, 2021
Copy link
Collaborator

@rvilarl rvilarl left a comment

Choose a reason for hiding this comment

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

Very good idea to separate this block, LGTM.
And as you mentioned totally safe to merge.

@rvilarl
Copy link
Collaborator

rvilarl commented Nov 6, 2021

@0xfe IMO this is ready to merge.

@ronyeh ronyeh mentioned this pull request Nov 10, 2021
Copy link
Owner

@0xfe 0xfe 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! Some very minor comments, but this is good to merge.

<script type="module">
console.log('VexFlow BUILD ' + Vex.Flow.BUILD);

////////////////////////////////////////////////////////////////////////
Copy link
Owner

Choose a reason for hiding this comment

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

Not a huge fan of this big comment line :-)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sounds good. Will remove. Sometimes I put in these "horizontal separators" to help guide the code reader between large code blocks, but here I have extra braces { } anyways, so they're probably not needed.

@ronyeh ronyeh closed this Nov 12, 2021
@ronyeh ronyeh deleted the font/demos branch January 18, 2022 22:40
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.

4 participants