-
Notifications
You must be signed in to change notification settings - Fork 9
Description
Issue
When a user chooses a highlighting color - that piece of data should be recorded into the styleClass prop.
In my app, I manually extended the TextAnnotationTarget type in a .d.ts file with styleClass. So I'm able to record it with store.updateAnnotation(merge({}, annotation, { target: { styleClass } })). Which gets correctly serialized for the W3C annos:
text-annotator-js/packages/text-annotator/src/model/w3c/W3CTextFormatAdapter.ts
Lines 141 to 146 in 3a70b9e
| return { | |
| ...targetRest, | |
| id: s.id, | |
| source, | |
| selector: w3cSelectors | |
| }; |
But, unfortunately, not having that type on the TextAnnotation itself - led to a parsing bug:
text-annotator-js/packages/text-annotator/src/model/w3c/W3CTextFormatAdapter.ts
Lines 42 to 48 in 3a70b9e
| const parsed: TextAnnotationTarget = { | |
| creator: parseW3CUser(creator), | |
| created: created ? new Date(created) : undefined, | |
| updated: modified ? new Date(modified) : undefined, | |
| annotation: annotationId, | |
| selector: [] | |
| }; |
The
styleClass from the w3c anno never ends up on the core model and we lose it... 😭
The question of whether we should have the styleClass on the core model has already been brought up previously. The decision at that point was to not add it because:
it should stay out of the core. In the longer run (hopefully not too far future...) both the
selectActionand thestylefunction would be affected by the adapter. Which means you would get aW3CTextAnnotationas an argument to yourstyle(orpointerAction) function.
Unfortunately, now I see a small flaw in that logic... If we don't have the styleClass somewhere in the Core model - there will nothing for the adapter to put into the W3CTextAnnotation 🤷🏻♂️
Possible Solution
- Add the
styleClass?: stringtype to theTextAnnotationmodel - Add copying of the
styleClassfrom theW3CTextAnnotationto theTextAnnotation