Handle Long Melismas + Add Musicologist Button #120
Conversation
Feat: Add button to download PDF for musicologists
|
@notkaramel, would you have some time to take a look at this PR once is finished? We would appreciate it very much. Thank you! :) |
|
@martha-thomae Of course! Hopefully by the end of this week. @DeannaLC Quick glance, I don't think having the Musicologist button to download a file is a good idea. Can you make it open a new tab to the file? It can be hosted on GitHub/Google Drive. Best thing to do is to package it with the build in a |
|
Got it, just added in the public/Test_PDF.pdf along with 1 other tiny fix! (Unfortunately can't test the link since the PDF isn't there yet, but think it should work and can update it if it doesn't) |
DeannaLC
force-pushed
the
cutoff-chart
branch
from
d9c57d7 to
86e1008
Compare
|
Sorry, keep adding on small final touches but believe that's everything now! @notkaramel |
notkaramel
left a comment
notkaramel
left a comment
There was a problem hiding this comment.
Looking good so far, I have some questions (see the reviews)
Let me know if you have any question :))
| const rootPamMeiDatabase = | ||
| "https://github.com/ECHOES-from-the-Past/PAM-MEI-Database/blob/main/"; | ||
| let fileName = chant.fileName; | ||
| let a = document.createElement("a"); | ||
| a.href = rootGABCtoMEI + fileName; | ||
| let chantLink; | ||
| if (chant.notationType == "square"){ | ||
| chantLink = "Square_MEIs/"; | ||
| } | ||
| else if (chant.notationType == "aquitanian"){ | ||
| chantLink = "Aquitanian_MEIs/"; | ||
| } | ||
| else if (chant.notationType == "old_hispanic"){ | ||
| chantLink = "Old_Hispanic_MEIs/"; | ||
| } | ||
| a.href = rootPamMeiDatabase + chantLink + fileName; |
There was a problem hiding this comment.
I think we can add a new field "meiUrl" to the Chant object to have it parsed in database.mjs, in case there's a database change again, we don't have to manually change the URL there.
It would also contain the file path, since chant.filename is a strip down string of the full path 😆
Feel free to make a new issue for this!
| callback: function(val, index) { | ||
| if (val == 20 && chant.notationType == "old_hispanic"){ | ||
| return "20+" | ||
| } |
There was a problem hiding this comment.
How come the cutoff is 20 but the condition is val == 20? Shouldn't it be val >= 20?
| <ExternalLink href="https://github.com/ECHOES-from-the-Past/PAM"> | ||
| <Button>Source Code</Button> | ||
| </ExternalLink> | ||
| <Download>For Musicologists</Download> |
There was a problem hiding this comment.
| <Download>For Musicologists</Download> | |
| <ExternalLink href="./Test_PDF.pdf"> | |
| <Button> Guide for Musicologists </Button> | |
| </ExternalLink> |
When testing, this might not work because the PDF is not part of the path, but for the actual website, it will be at https://echoes-from-the-past.github.io/PAM/Test_PDF.pdf
And you can remove the Download component.
If you still want it to be pink, you can make a $prop() for Tailwind classes and set the green ones to be the default so it doesn't break the project, and pink can be added as an option.
| rhombusBg: "#3c74f9aa", | ||
| rhombusBorder: "#1310f9aa", | ||
| ncBg: "#9270f6aa", | ||
| ncBorder: "#9253f2aa" |
There was a problem hiding this comment.
These are different hexcodes from the first screenshot you sent. Do you have a screenshot for the ncBg and ncBorder? I couldn't test that one.
Personally, the rhombus color is a bit harsh compared to the regular note color - it was pastel green and cyan when I implemented it. It's perfectly fine as you implemented it, just my personal preference :)))
| labels, | ||
| datasets: [ | ||
| { | ||
| label: "Number of NCs", |
There was a problem hiding this comment.
Is "NCs" understandable for musicologists? i.e., do they also call it "neume component", or just "neume" or "note"?
Think this is the last commit I've got for the time being! Did an implementation of the chart cutting off to deal with longer melismas as mentioned by Elsa here. Additionally added in patterns for differentiation with the rhombus and 20+ NC bars.
Also added in a button to download a PDF which will have a write-up for musicologists working with PAM - currently a placeholder and will be filled in later.