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

Allow for displaying only traditional music notation, without tablature #8

Closed
wants to merge 4 commits into from

Conversation

adamf
Copy link
Contributor

@adamf adamf commented Feb 21, 2011

These changes allow the user to display only traditional music notation by adding a "tablature=false" option to the tabstave tag. This is implemented by adding a new state that works with has_notation, has_tablature, and wrapping a few key pieces of code in vextab, tabdiv & the vexflow formatter in a conditional which checks has_tablature.

This allows the user to render only notation, not tablature and notation.
@0xfe
Copy link
Owner

0xfe commented Feb 22, 2011

Ignore that last comment (I missed the second commit).

Thanks for the change. Overall it looks good. Do you mind adding a test that validates the parse error?

@adamf
Copy link
Contributor Author

adamf commented Feb 22, 2011

Gladly! How do I run the test infrastructure? I'd have written one but didn't know how to use the tests you've build, and bringing up the one HTML file in the tests/ directory didn't seem to do anything.

I also notice that some simply notations don't render, even with the stock tabdiv-free. I'll open a issue with test cases.

@0xfe
Copy link
Owner

0xfe commented Feb 22, 2011

I noticed that the tests in vextab/ were broken. I just fixed them - so you should be able to open vextab/runtest.html and see all tests pass (after pulling in the most recent changes.)

Adding unit tests for the new notation-only mode:
1) render with both tablature and notation
2) render with just notation
3) test that we get a parseError when both tablature and notation are false
@adamf
Copy link
Contributor Author

adamf commented Feb 23, 2011

I've added tests - take a look.

@0xfe
Copy link
Owner

0xfe commented Feb 23, 2011

Looks good. Thanks for the change.

This pull request was closed.
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