send content path #61
Conversation
src/SerializedHighlight.ts
Outdated
|
|
||
| let contentPath: number[] | undefined; | ||
|
|
||
| if (serializationData.type === xPathDiscriminator) { |
There was a problem hiding this comment.
can we not limit this to just one serialization strategy? you have access to the dom nodes here you should be able to get everything you need
There was a problem hiding this comment.
I'm testing out some changes to this to use text position. I'm not sure if there's a case where the strategies could get mixed, but my guess is no, and that it's safe to say comparisons would always be the same type for a given reference id. Since xpath is the default it's hard for me to understand how text position gets used in real content (though how it is represented is very simple.)
There was a problem hiding this comment.
Okay I checked today and it looks like Tutor is using Xpath not TextSelector, so I'm not sure of a "real" test. But I think this approach here will work, since the highlighted text parent should always be the reference element, so just return 0 and the offset: 6cf0fd3#diff-db95139714bd7e6e07fffb3e5099c11f8fcb08d2263985401647db6d2c6e8f15R10
package.json
Outdated
| "tslint": "^5.11.0", | ||
| "tslint-loader": "^3.6.0", | ||
| "typescript": "^3.1.1", | ||
| "typescript": "3.5", |
There was a problem hiding this comment.
updating typescript is fine, but was there a reason to do it as part of this change?
There was a problem hiding this comment.
I was running into these errors when building: Argument of type 'string' is not assignable to parameter of type 'NewHighlightColorEnum' and Cannot find name 'GlobalFetch'. Possibly something messed up with my setup if it was building fine before though 🤔
|
Some of the build errors will go away once the client types are updated to production, but there are some others I'm not sure of the cause from, I'll look into those. |
|
@jivey is this ready for review? a bunch of tests are failing |
|
@TomWoodward yes meant to ping you after changing it to use a simple path for text strategy. The tests are failing because of the highlights-client, maybe we can pair on that soon because I can't figure out why it isn't building... I ran this locally without publish and linked it to highligher: https://github.com/openstax/ox-bin/blob/main/lib/build-swagger-client and it fixed the GlobalFetch error, but I still get |
No description provided.