-
Notifications
You must be signed in to change notification settings - Fork 70
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
feat: use data uri for card fallback image #3323
feat: use data uri for card fallback image #3323
Conversation
✅ Deploy Preview for paragon-openedx ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, that was fast! Thanks :)
I agree "no image at all" would be better, but this is a nice improvement for now.
So this is failing on paragon/src/Card/tests/CardImageCap.test.jsx Lines 122 to 130 in 1b90fda
now, but I'm not sure what "hiding component if it isn`t fallbackSrc and src don`t work" is supposed to be testing, or if it's accomplishing its intended purpose. It was passing before because the fallback png would lead to Line 1 in 1b90fda
I tried throwing jest.mock('../CardFallbackDefaultImage', () => ({ cardSrcFallbackImg: 'test-file-stub' })); into the test file to see if I could fix the error really quickly, but it still failed. Before spending any real amount of time fighting this test I'd like to understand what it is actually trying to verify. |
@PKulkoRaccoonGang I checked the git blame on the test and it looks like you added it last year as part of #1994. Any chance you remember what expect(srcImg.src.endsWith('test-file-stub')).toEqual(true); is verifying? |
@brian-smith-tcril, thank you for these changes! In the package.json, we have the following configuration: "moduleNameMapper": {
"\\.(jpg|jpeg|png|gif|eot|otf|webp|svg|ttf|woff|woff2|mp4|webm|wav|mp3|m4a|aac|oga)$": "<rootDir>/__mocks__/fileMock.js",
"\\.(css|scss)$": "identity-obj-proxy"
} This setup is necessary because Jest does not natively handle media files. Instead, a stub is provided to prevent errors during test execution. We use this approach in the Avatar component, as it requires mocking a .svg image. expect(srcImg.src.endsWith('test-file-stub')).toEqual(true); This check verifies that the mocked file (test-file-stub) is used as the source for the component’s src attribute when the actual image loading fails. Since we now want to use an image in base64 format, there is no longer a need to mock the image. I propose updating the test as follows: it('renders the default image if both src and fallbackSrc fail to load', () => {
render(<CardImageCapWrapper src="fakeURL" fallbackSrc="fakeURL" srcAlt="Src alt text" />);
const srcImg = screen.getByAltText('Src alt text');
fireEvent.load(srcImg);
fireEvent.error(srcImg);
expect(srcImg.src).toEqual(cardSrcFallbackImg);
}); I tested this change locally, and it works as expected. |
5f23498
to
7d270e0
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## release-22.x #3323 +/- ##
================================================
+ Coverage 93.31% 93.57% +0.25%
================================================
Files 249 259 +10
Lines 4441 4635 +194
Branches 1014 1077 +63
================================================
+ Hits 4144 4337 +193
+ Misses 294 292 -2
- Partials 3 6 +3 ☔ View full report in Codecov by Sentry. |
Thanks for the advice @PKulkoRaccoonGang! I've updated the test and everything looks good now! |
trying close/reopen to trigger the required deploy preview (switched base from |
🎉 This PR is included in version 22.12.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
🎉 This PR is included in version 23.1.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Description
Deploy Preview
Include a direct link to your changes in this PR's deploy preview here (e.g., a specific component page).
Merge Checklist
example
app?Post-merge Checklist