fix: use slice instead of substr for stripping "expandable" from alt text #52
Conversation
of course, will do! |
|
@mgc1194 @stephenliang @Hamms does anyone know if I need to do a version bump here and update the main repo's package json to pull in this change? |
There was a problem hiding this comment.
Always great to see accessibility improvements! 🎉
I have one tiny nit about an implementation detail, but otherwise this change looks great! Love the testing, too.
does anyone know if I need to do a version bump here and update the main repo's package json to pull in this change?
Yep! Let me know if you need any npm permissions to do that version bump
src/expandableImages.js
Outdated
| link.alt && | ||
| link.alt.endsWith("expandable") | ||
| ) { | ||
| const altText = link.alt.replace(/expandable$/, "").trim() |
There was a problem hiding this comment.
FWIW, although this implementation is definitely easier to read than what we had before, the original implementation is approximately 50 times faster than this one (according to a quick https://jsbench.me/ test).
Because this is a low-level utility which is distributed without transpilation of any kind, little optimizations like this can end up mattering a lot more than they would in our primary codebase; I think we want to preserve the original. Might also be worth adding a comment explicitly calling out why we're prioritizing performance over readability here!
There was a problem hiding this comment.
ah interesting! TIL regex is quite slow 😬
there is a problem with the original implementation though, the substr method's second argument is the length of the string that is returned. passing a negative number will return an empty string every time. I'm sure it was meant to be slice instead though
There was a problem hiding this comment.
Lol, yeah; you don't get that kind of power and flexibility without sacrificing performance. C'est la vie!
And oh dang, good catch! You're absolutely right that this was meant to be slice. I guess that's what we get for not having had unit tests on this before 😅
test/utils.js
Outdated
| // Add support for expandableImages | ||
| schema.tagNames.push('span'); | ||
| schema.attributes.span = ['dataUrl', 'className']; | ||
| schema.attributes.span = ['dataUrl', 'dataAlt', 'className']; |
There was a problem hiding this comment.
I think you'll also want to apply this change here over in the main repo PR
There was a problem hiding this comment.
ooo, thank you! the more I think about it, since the original implementation passed the alt text as the text content of the span, maybe I can grab it from there in the main repo, rather than adding a data attr 🤔
I'm going to try that, so there are minimal changes here. just the bug fix and the tests
There was a problem hiding this comment.
That sounds like a good approach to me 👍
drizco
force-pushed
the
ryan/remark/feat/pass-alt-text-as-span-data-attribute
branch
from
7d667e8 to
a9b5f64
Compare
drizco
force-pushed
the
ryan/remark/feat/pass-alt-text-as-span-data-attribute
branch
from
a9b5f64 to
1dd5309
Compare
|
@Hamms updated with the bug fix, tests, and a patch version bump. after this merges I'll try |
|
This repo is a candidate for moving to the main repo via the modularization project but I would not block on that work. I would do the version bump, publish, and pull down for now. |
drizco
changed the title
5 participants
This pull request fixes a bug when parsing the alt text for expandable markdown. The original implementation used
String.substrwhen I believe the intention was to useString.slice.substris actually deprecated, and first argument is the starting index, and the second argument is the length of the returned string. Passing a negative number as the second argument is not supported and will default to zero, so an empty string is returned. from mdn:If length < 0, an empty string is returned..slicefirst arg is the starting index, and the second arg is the ending index, which does support negative numbers and will track back from the end of the string. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/sliceunit tests have been added to confirm the bug and the fix.
version has been updated in package.json to increment the patch value.