search

DCMLab / reductive_analysis_app

Web app for reductive analyses of scores
https://dcmlab.github.io/reductive_analysis_app/
Other
15 stars 4 forks source link

Xml refactor corresp #191

Closed pettter closed 2 years ago

pettter commented 2 years ago

This changes the behaviour of the app to use @corresp attributes instead of @sameas/@copyof in both layers and analyses, and adds a function to fix files created with the old method. This fixes #162 .

meduzen commented 2 years ago

Seems okay to me, looking at the code. As it relates to the MEI spec, maybe @yrammos is the right person to validate this PR (?).

yrammos commented 2 years ago

@pettter thanks for this. The code looks clean and works well in informal testing. xmlns="" attributes are added to all <graph> elements, and I don't think this is intentional(?)

UPDATE: diff screenshot attached.

diff

pettter commented 2 years ago

Huh, no you're right that's not intentional. Interestingly, I don't seem to be getting the same issue somehow.

I'm running on node 14.18.1 in Firefox 97.0

yrammos commented 2 years ago

Node 14.18.0 on Safari 15.3 here. More likely a WebKit than a node thing, right?

pettter commented 2 years ago

Yes, I can't seem to find anything about it on a quick google, but it's also not the most easy thing to search for.

Could you check if it does the same thing also on the ui branch, pre-this branch?

yrammos commented 2 years ago

It does, actually. It adds the tag to the <respStmt/> scope and also to <label> tags — curiously, though, not all of them this time. Screenshots attached.

Screen Shot 2022-02-22 at 15 48 54 Screen Shot 2022-02-22 at 15 48 49

yrammos commented 2 years ago

Might we need to sanitize our output from such dummy attributes?

pettter commented 2 years ago

I think it might be a case of Safari being a bit of a pedant and actually looking at the linked standard and then adding the @xmlns attribute for any nodes that are not in the stated namespace.

In any case, yes it's an issue that should be addressed, but not by this PR, I don't think.

yrammos commented 2 years ago

In that case I'm closing this and merging. Thanks, @pettter .