Add support for about attributes when parsing rdfa relationships
#1052
Conversation
| } | ||
| if (type === 'resource') { |
There was a problem hiding this comment.
I know this is already merged, but I didn't notice before
so if I understand correctly this is making sure resource nodes have a resource attribute, even if there is none in the dom, right?
It's a bit janky cause it relies on the ordering of the rdfaDomAttrs constant, and Object.keys looping over the keys in order (which is probably in the spec, but still it's a bit fragile)
I'd do these checks after the loop to make it a bit more clear
There was a problem hiding this comment.
I think you're misreading the ordering. The idea here is that if the rdfaNodeType is resource it will make sure that the resource and properties fields are defined on the attrs object. If they have already been set it will keep them the same, if not, it will set them to '' or []. This is just to avoid getting undefined when attempting to access them, and to conform to the stricter types that I added. I'll add a note about what it's doing.
There was a problem hiding this comment.
ah yeah it would indeed be fine cause we don't overwrite when there's no value, so even if we hit resource later in the loop we would either overwrite it with the right value, or not do anything
I'd still put it outside of the loop personally, but its preference
There was a problem hiding this comment.
Fair. The typing is easier this way though, so I'll leave it here. As soon as I set rdfaNodeType to be resource, it insists that the others are present.
addon/core/schema.ts
Outdated
| }; | ||
| case 'about': { | ||
| const about = value as unknown as string; | ||
| const property = node.attributes.getNamedItem('property')?.value; |
There was a problem hiding this comment.
I'm concerned this is necessary. The idea is that we run the html through a spec-compliant rdfa parser, which "should" be able to generate the correct backlinks and properties, so by the time it gets to this logic we shouldn't have to worry about it anymore, can you elaborate on what's going wrong here?
(there's a good chance our parsing logic is wrong, but then I'd rather fix it there)
There was a problem hiding this comment.
Hmm, so the issue I'm seeing is that if I insert a structure, say a chapter, then the chapter resource has heading and title properties, each of which have the correct backlinks. If I then refresh, the heading is no longer correctly linked to the chapter (either the property on the chapter or the backlink on the heading.
It may well be that there is an issue with the parser that means it's missing these relations (they're in the DOM with an about and property attribute on the heading.
There was a problem hiding this comment.
Since you've confirmed that there is a parser issue, I'll wait for that change, confirm that it's fixed, and remove this from the PR when done. This PR will then just be an improved comment...
piemonkey
force-pushed
the
rdfa/rdfa-about
branch
from
82fec57 to
7e89d21
Compare
|
I'm going to merge this now since it's just a comment. |
Overview
Adds
aboutto the parsing of rdfa to establish properties and backlinks.connected issues and PRs:
lblod/ember-rdfa-editor-lblod-plugins#358
Setup
Easiest used with the PR from plugins with article structure plugin changes.
How to test/reproduce
In the latest version of that PR, look at the properties and backlinks for
say:headingnodes. After refresh, these were not correctly maintained, this PR corrects that.Challenges/uncertainties
Checks PR readiness