-
Notifications
You must be signed in to change notification settings - Fork 33
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: video gallery thumbnail fallback #412
feat: video gallery thumbnail fallback #412
Conversation
Thanks for the pull request, @ArturGaspar! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
1468405
to
1e6fb33
Compare
Hi @openedx/teaching-and-learning! This is ready for review. |
thumbnail | ||
className="px-6 py-4.5" | ||
src={videoThumbnail} | ||
style={{ height: '100%' }} |
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.
Maybe we can use https://paragon-openedx.netlify.app/foundations/css-utilities for the classes from here.
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.
👍
- ✅ I tested this on devstack and it looks good to me
- ✅ I read through the code
- ❌ I checked for accessibility issues
- ❌ Includes documentation
- ❌ I made sure any change in configuration variables is reflected in the corresponding client's
configuration-secure
repository.
1e6fb33
to
9b9ea79
Compare
@openedx/teaching-and-learning - this is ready for review / merge. Thanks! |
@navinkarkera Another PR to tag with "jira:2u" label. cc: @cablaa77 |
I've created issue TNL-11196 in the private 2U Jira. |
<Image | ||
thumbnail | ||
className="px-6 py-4.5 h-100" | ||
src={videoThumbnail} | ||
/> |
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.
Why is image used instead of Paragon's <Icon />
component with a Paragon icon?
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.
@KristinAoki The VideocamOutlined icon was added in Paragon v21.6.0 (47e4379).
The latest release on https://www.npmjs.com/package/@edx/paragon is 21.5.6 and a notice says that future versions will be publised as https://www.npmjs.com/package/@openedx/paragon.
Installing @openedx/paragon and importing VideocamOutlined from it works, but is it a good idea?
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.
I would probably wait for the whole repo to switch over to @openedx/paragon. Consider this resolved.
@ArturGaspar 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
Description
Add a fallback when thumbnail fails to load in video gallery.
Before:
After:
Testing instructions
contentstore.mock_video_uploads
enabled for everyone)new_core_editors.use_new_video_editor
andnew_core_editors.use_video_gallery_flow
enabled for everyone)