Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix losing the target.styleClass property on the W3CTextAnnotation parsing #71

Open
oleksandr-danylchenko opened this issue Feb 29, 2024 · 0 comments

Comments

@oleksandr-danylchenko
Copy link
Contributor

oleksandr-danylchenko commented Feb 29, 2024

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:

return {
...targetRest,
id: s.id,
source,
selector: w3cSelectors
};

But, unfortunately, not having that type on the TextAnnotation itself - led to a parsing bug:

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... 😭
image


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 selectAction and the style function would be affected by the adapter. Which means you would get a W3CTextAnnotation as an argument to your style (or pointerAction) 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

  1. Add the styleClass?: string type to the TextAnnotation model
  2. Add copying of the styleClass from the W3CTextAnnotation to the TextAnnotation
oleksandr-danylchenko added a commit to oleksandr-danylchenko/text-annotator-js that referenced this issue Feb 29, 2024
oleksandr-danylchenko added a commit to oleksandr-danylchenko/text-annotator-js that referenced this issue Mar 27, 2024
oleksandr-danylchenko added a commit to oleksandr-danylchenko/text-annotator-js that referenced this issue Apr 4, 2024
# Conflicts:
#	packages/text-annotator/src/highlight/canvas/highlightRenderer.ts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant