On 13 Jul 2015, at 20:10, Randall Leeds wrote: > > Fragment Selector: https://github.com/hypothesis/dom-anchor-fragment > Text Position Selector: > https://github.com/hypothesis/dom-anchor-text-position > Text Quote Selector: https://github.com/hypothesis/dom-anchor-text-quote > > All of these have been published on npm as alpha versions, which means you > have to specify the version explicitly when installing them for now. > > I would appreciate any feedback on these and I hope that they are useful > for implementations or discussion. I've had a look through all three of these, and overall I think they look great! Simple, nice. Lovely. Only one question and a couple of comments: - what's the basis for the 32 character prefix/suffix for TextQuoteAnchor? I'm sure it's fine but regardless of whether it is a magic number of there's more to it, it should be a constant, not inlined in a couple of places through the code. - a handful of judiciously placed comments would help with code comprehension -- particularly in the TextPositionAnchor and to a lesser extent the TextQuoteAnchor code. - couple of minor typos (link syntax) in the dom-anchor-text-position README. -N
Received on Wednesday, 15 July 2015 05:23:02 UTC