New approach to svg-image #1983
Conversation
SonicScrewdriver
changed the title
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (2152613) and published it to npm. You Example: yarn add @khanacademy/perseus@PR1983If you are working in Khan Academy's webapp, you can run: ./dev/tools/bump_perseus_version.sh -t PR1983 |
|
Size Change: -246 B (-0.02%) Total Size: 1.27 MB
ℹ️ View Unchanged
|
| const svgLocalLabelsRegex = /^file\+graphie:/; | ||
| const hashRegex = /\/([^/]+)$/; | ||
|
|
||
| /** |
There was a problem hiding this comment.
These types needed to come along to support the parsing. I'm exporting them for Graphie2000, but I do wonder which side should be in charge of these.
There was a problem hiding this comment.
I think it makes sense for Perseus to own them. They are a part of how SvgImage works and consumes graphie images.
|
|
||
| // also test the array of edge cases from the testdata file | ||
| edgeCases.forEach((edgeCase, i) => { | ||
| it(`should parse the edge case ${i}`, () => { |
There was a problem hiding this comment.
There's a lot of edge cases, and some of them are quite similar. I could reduce them down, potentially, but I did like that we could cover 99% of the cases outlined in the ticket as failing to parse. Happy to take feedback on this one!
There was a problem hiding this comment.
I think the collection of edge cases you have is probably good. The ones that will fail (like that one with a u'' prefix on all strings, is likely broken in prod too so I'd guess it would need to be re-written to work anyways.
| }, | ||
| }); | ||
| }); | ||
| } |
There was a problem hiding this comment.
I understand that this is simply copy-pasted from the svg-image.tsx file, but I think a bit of simplification might be helpful here. Perhaps the shouldUseLocalizedData() conditional block could be written as follows:
const dataErrorHandler = (x, status, error) => {
Log.error("Data load failed for svg-image", Errors.Service, {
cause: error,
loggedMetadata: {
dataUrl: Util.getDataUrl(url),
status,
},
});
if (shouldUseLocalizedData()) {
retrieveData(getLocalizedDataUrl(url), (x, status, error) => {
cacheData.localized = false;
// If there is isn't any localized data, fall back to
// the original, unlocalized data
retrieveData(Util.getDataUrl(url), dataErrorHandler)
} else {
retrieveData(Util.getDataUrl(url), dataErrorHandler)
}
| const svgLocalLabelsRegex = /^file\+graphie:/; | ||
| const hashRegex = /\/([^/]+)$/; | ||
|
|
||
| /** |
There was a problem hiding this comment.
I think it makes sense for Perseus to own them. They are a part of how SvgImage works and consumes graphie images.
|
|
||
| // also test the array of edge cases from the testdata file | ||
| edgeCases.forEach((edgeCase, i) => { | ||
| it(`should parse the edge case ${i}`, () => { |
There was a problem hiding this comment.
I think the collection of edge cases you have is probably good. The ones that will fail (like that one with a u'' prefix on all strings, is likely broken in prod too so I'd guess it would need to be re-written to work anyways.
| // Assert | ||
| expect(container).toMatchSnapshot(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
This is a great set. I do think it'd be good to add one or two tests to cover the path of loading a localized Graphie. There's branching in there where it tries to load the localized version and falls back to en if it fails. I think if we add a test for localized loading and the fallback, that'd be awesome.
…ere seemed appropriate.
SonicScrewdriver
force-pushed
the
jer/lems-2524-svg-image
branch
from
9aae0de to
2152613
Compare
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @khanacademy/[email protected] ### Minor Changes - [#1988](#1988) [`cc9d3a4bc`](cc9d3a4) Thanks [@SonicScrewdriver](https://github.com/SonicScrewdriver)! - Hiding graphie labels from screen readers. - [#1983](#1983) [`2748a1ff8`](2748a1f) Thanks [@SonicScrewdriver](https://github.com/SonicScrewdriver)! - Updating how svg-image loads data ## @khanacademy/[email protected] ### Patch Changes - Updated dependencies \[[`cc9d3a4bc`](cc9d3a4), [`2748a1ff8`](2748a1f)]: - @khanacademy/[email protected]
Summary:
This PR updates our approach to loading graphie images for svg-image.
An error will be thrown if the labels are not found, but should not impede users. Many tests were added using the failed cases provided on the ticket, in order to ensure that this new approach works for nearly all use cases.
Issue: LEMS-2524
Test plan: