Update article structure plugin to support rdfa editing #358
Conversation
There was a problem hiding this comment.
The different article structures seem to be well integrated with the new rdfa-system already :) Very nice work!! I have added some comments with questions/remarks.
Some additional questions/remarks:
- There still seem to be some issues with selections in and around structures, some of which also are related to the gap-cursor. We should look into improving that behaviour.
- (not new) Pressing
enterin a title splits the node in two, ideallyentershould do nothing. - The different article structures have each two attribute-properties (heading and number), for which I'm not sure if these are (still) needed.
- (design-related, not specific to this PR): we should probably make it clearer which parts of the structures are editable, and which are not (e.g. with different backgrounds, different cursors). Probably needs design input.
- We could look into leveraging the
propertiesandbacklinksattributes in the parseDOM-rules in order to parse the different structures.
| { | ||
| type: 'attribute', | ||
| predicate: ELI('number').prefixed, | ||
| object: numberConverted, | ||
| }, | ||
| { | ||
| type: 'attribute', | ||
| predicate: SAY('heading').prefixed, | ||
| object: `Artikel ${numberConverted}: ${titleText}`, |
There was a problem hiding this comment.
What is the reason that these are still added as attribute-properties? As their content would also be included in the title-node right?
There was a problem hiding this comment.
This was my first idea of how to handle headings and titles with almost the same content, but I've now got a better version with them both as external properties. I'll add this to the PR soon now that I'm confident that the current weird issue I'm investigating isn't actually caused by this change.
There was a problem hiding this comment.
I've pushed a version with heading split out, I'll put one in with number as an external tomorrow.
| { | ||
| type: 'attribute', | ||
| predicate: ELI('number').prefixed, | ||
| object: numberConverted, | ||
| }, | ||
| { | ||
| type: 'attribute', | ||
| predicate: SAY('heading').prefixed, | ||
| object: `${numberConverted}. ${titleText}`, | ||
| }, |
| export const structure_header_title: NodeSpec = { | ||
| attrs: { | ||
| ...rdfaAttrSpec, | ||
| __tag: { default: 'span' }, |
There was a problem hiding this comment.
I don't think this attribute is necessary anylonger.
There was a problem hiding this comment.
Okay, I'll remove it.
| }, | ||
| parseDOM: [ | ||
| { | ||
| tag: 'div', | ||
| priority: 55, |
There was a problem hiding this comment.
Could you add a comment as to why this priority is added/needed?
There was a problem hiding this comment.
I removed this one actually as it wasn't necessary...
| ) { | ||
| return { resource: element.getAttribute('resource') }; | ||
| const resource = element.getAttribute('resource'); | ||
| if (hasRDFaAttribute(element, 'typeof', type) && resource) { |
There was a problem hiding this comment.
I suppose we could do this check on the properties attribute of the incoming html-element now?
e.g.:
attrs.properties.some((prop) => prop.predicate === 'typeof' && prop.object.resource === type)There was a problem hiding this comment.
I'm not sure what you mean. Do you mean to use getRdfaAttrs() to process the element and use the properties member of that attrs object?
There was a problem hiding this comment.
at the point prosemirror gets the html, it is already preprocessed to contain the properties and backlinks
| parseDOM: [ | ||
| { | ||
| tag: 'h1,h2,h3,h4,h5,h6,span', | ||
| priority: 60, |
There was a problem hiding this comment.
Could you add a comment on the reason of this priority?
There was a problem hiding this comment.
note the schema you define for a doc is also priority-ordered, so if we put the structure nodes before the headers, it should also parse correctly with default prio
(gives more flexibility to the host app)
| priority: 60, | ||
| getAttrs(element: HTMLElement) { | ||
| const level = TAG_TO_LEVEL.get(element.tagName.toLowerCase()) ?? 6; | ||
| const property = element.getAttribute('property'); |
There was a problem hiding this comment.
We could do this check with the properties and backlinks attributes now I think.
There was a problem hiding this comment.
Again, I guess you mean rdfa attributes? So using the processed relationships rather than relying on specific html attributes?
There was a problem hiding this comment.
I think we can also update the parseDOM of these nodes to leverage the properties and backlinks. The toDOM should also be updated to use the renderRdfaAware function. I'm also not sure if it will be necessary to keep all these seperate node-specs.
There was a problem hiding this comment.
I saw this as a separate task, the only reason I changed these at all was to update the name rdfaAttrs to rdfaAttrSpec to reflect that the constant holds values in the format of a spec ({ [name]:{ default: 'value' } }), rather than in the format of attributes ({ [name]: 'value' }) as that wasn't clear from the original name.
Overview
Correctly support the insertion of any structures in the article structures plugin.
All comments should have been addressed, with the exception of pressing enter in the titles, which still creates a whole new structure with a new title. There are some design issues, but since we'll need to overhaul the design for editable nodes, I haven't done any tweaking.
connected issues and PRs:
This is made towards #357 with the intent that it collects all of the related changes rather than them being merged straight to master.
PR on editor repo
Setup
The 'editable nodes' route should now have the article structure plugin enabled.
How to test/reproduce
Inserting structure elements should function as expected.
Challenges/uncertainties
Checks PR readiness