Add fallback image for the course card images #324

hinakhadim · 2024-04-22T13:54:50Z

When there is no image for course, then the screen shows alt text with broken image icon. This PR fixes it by adding an error image url. The Error image needs to be added in edx-platform with name no_course_image.png. This works perfectly with even tutor-indigo or any theme you want to set using tutor.

Before:

After:

If the image exists in theme:

If the image with name no_course_image.png won't exist, then it will show the alt text with broken image icon.

Impact:

It will enhance user experience
It'll maintain consistency of the course card layout
Easy to implement with tutor-indigo theme or any customizable theme. Just need to add image with that name.

justinhynes · 2024-04-23T16:43:52Z

src/containers/CourseCard/components/CourseCardImage.jsx

  const { bannerImgSrc } = reduxHooks.useCardCourseData(cardId);
  const { homeUrl } = reduxHooks.useCardCourseRunData(cardId);
  const { isVerified } = reduxHooks.useCardEnrollmentData(cardId);
  const { disableCourseTitle } = useActionDisabledState(cardId);
  const handleImageClicked = reduxHooks.useTrackCourseEvent(courseImageClicked, cardId, homeUrl);
  const wrapperClassName = `pgn__card-wrapper-image-cap overflow-visible ${orientation}`;
+  const fallbackImageSrc = `${getConfig().LMS_BASE_URL}/theming/asset/images/no_course_image.png`;


Is the no_course_image.png asset included by default as part of edx-platform? I was trying to check now and I can't find any assets in the master branch with this name, but maybe I am missing it.

Ah, re-reading your description now, I believe you're saying that the Open edX operator must provide an asset with this name for this feature to work?

I'm a little nervous that this feature would be a bit hard to discover. I think it would be better for operators if there were some standard theme-agnostic placeholder images available in edx-platform that we're referencing.

There is precedent for hosting a filler image in the MFE itself in case of absence:

frontend-app-learner-dashboard/src/containers/CoursesPanel/NoCoursesView/index.jsx

Line 7 in d3779ad

import emptyCourseSVG from 'assets/empty-course.svg';

I'm not saying I prefer this, architecturally speaking, but it would be a more immediate fix than adding placeholder images to edx-platform.

Also, I don't think hard-cording a reference to an image that needs to be provided as part of a comprehensive theme (something that's likely going away in the future), is the right thing to do here. If we're hard-coding something, it needs to be part of the vanilla edx-platform theme.

justinhynes · 2024-04-23T16:44:25Z

Hi @mphilbrick211 -- Is this something that would need product eyes from Axim?

justinhynes

Blocking momentarily while it is determined if we need further feedback from Axim product folks.

mphilbrick211 · 2024-04-23T20:05:57Z

Hi @mphilbrick211 -- Is this something that would need product eyes from Axim?

Thanks for flagging, @justinhynes - will check with Jenna now.

jmakowski1123 · 2024-04-24T17:04:55Z

This is a good fix, thanks! Will this default image work for any instance? I noted you mentioned it works with the indigo theme, will it work for anyone using tutor?

This has product approval.

hinakhadim · 2024-04-25T06:59:14Z

Yes, it works with indigo theme because in indigo theme, no_course_image.png is added. It won't work for anyone else using tutor without indigo theme because the image won't exist. Anyone who want to make this thing work has to add the image with no_course_image.png name in tutor theme package or edx-platform (directly).

justinhynes · 2024-04-29T13:03:52Z

@hinakhadim What is the behavior if the asset doesn't exist in an alternate theme? Does it fallback to the current behavior (showing a broken image?) or something worse (hopefully not an error?)

hinakhadim · 2024-05-02T04:57:15Z

@hinakhadim What is the behavior if the asset doesn't exist in an alternate theme? Does it fallback to the current behavior (showing a broken image?) or something worse (hopefully not an error?)

Yes, it fallback to the current behavior (showing a broken image). I've tested with image not existing then it didn't throw error and showed the current behavior.

arbrandes · 2024-05-02T15:53:04Z

src/containers/CourseCard/components/CourseCardImage.jsx

  const { bannerImgSrc } = reduxHooks.useCardCourseData(cardId);
  const { homeUrl } = reduxHooks.useCardCourseRunData(cardId);
  const { isVerified } = reduxHooks.useCardEnrollmentData(cardId);
  const { disableCourseTitle } = useActionDisabledState(cardId);
  const handleImageClicked = reduxHooks.useTrackCourseEvent(courseImageClicked, cardId, homeUrl);
  const wrapperClassName = `pgn__card-wrapper-image-cap overflow-visible ${orientation}`;
+  const fallbackImageSrc = `${getConfig().LMS_BASE_URL}/theming/asset/images/no_course_image.png`;


There is precedent for hosting a filler image in the MFE itself in case of absence:

frontend-app-learner-dashboard/src/containers/CoursesPanel/NoCoursesView/index.jsx

Line 7 in d3779ad

import emptyCourseSVG from 'assets/empty-course.svg';

I'm not saying I prefer this, architecturally speaking, but it would be a more immediate fix than adding placeholder images to edx-platform.

Also, I don't think hard-cording a reference to an image that needs to be provided as part of a comprehensive theme (something that's likely going away in the future), is the right thing to do here. If we're hard-coding something, it needs to be part of the vanilla edx-platform theme.

hinakhadim · 2024-05-08T12:15:03Z

@arbrandes agree to your point.

import emptyCourseSVG from 'assets/empty-course.svg';

This image is looking weird to fix the broken image.

If you can share other alternative, then i'll really appreciate it.

arbrandes · 2024-05-09T12:20:24Z

If you can share other alternative, then i'll really appreciate it.

Apologies, I didn't mean to say we should use that image. We'll need to create a new one that works for this use case.

hinakhadim · 2024-05-10T12:53:18Z

Sure. Can you please mention the team who will take care of this?

arbrandes · 2024-05-15T15:58:34Z

Good question. I guess I was expecting you'd be able to submit the one in Indigo here, but of course, that is presuming a lot. :)

If that's not the case, then it's just something we'll have to add to the community backlog for somebody to pick up (including, possibly, Axim itself, but I can't promise we'll have the capacity soon).

hinakhadim · 2024-05-17T06:00:34Z

Good question. I guess I was expecting you'd be able to submit the one in Indigo here, but of course, that is presuming a lot. :)

If that's not the case, then it's just something we'll have to add to the community backlog for somebody to pick up (including, possibly, Axim itself, but I can't promise we'll have the capacity soon).

We have included this image in indigo theme and add its link here in fallbackImgSrc variable. I know it makes us dependent on edx-platform but user can replace that image from theme and can use his own image (user is not restricted) .
Other option is that we can include that image here in this MFE and import it like import FallbackCourseImg from 'assets/no_course_img.png';. But, in this user must has to use that image (user is restricted). Otherwise, he has to change image by forking this repo. I am thinking from this perspective.

What are your thoughts on this regarding architecture considering future updates in LMS and MFE and user independence? (As studio/CMS also moved to MFE)

arbrandes · 2024-05-17T16:23:32Z

Other option is that we can include that image here in this MFE and import it like import FallbackCourseImg from 'assets/no_course_img.png';. But, in this user must has to use that image (user is restricted). Otherwise, he has to change image by forking this repo. I am thinking from this perspective.

Good point. This means we should make the image overridable via brand-openedx, or, as mentioned earlier, we make the image available as part of the default Open edX theme in edx-platform, so that users don't have to have indigo installed for this to work.

I prefer the first option, as it decouples the UI from the backend a little bit more. But it would be best if the image were provided as an SVG, as opposed to a PNG, if at all possible.

hinakhadim · 2024-05-21T06:34:52Z

Awesome. Including image in brand-openedx seems the best option because users can fork brand-openedx for image replacement instead of learner-dashboard mfe. I am able to include png image and that is working fine. If I import svg image, I am getting the following error:

Module not found: Can't resolve 'react' in '/openedx/brand-openedx/paragon/images'
brand-openedx/paragon/images/no_course_image.svg

jsnwesson · 2024-05-21T16:14:23Z

@hinakhadim unless I'm misunderstanding how brand-openedx is being used here, I don't see an SVG version of no_course_image in the repo, so that might explain why it's failing to import? https://github.com/openedx/brand-openedx/tree/master/paragon/images

hinakhadim · 2024-05-22T06:37:43Z

@hinakhadim unless I'm misunderstanding how brand-openedx is being used here, I don't see an SVG version of no_course_image in the repo, so that might explain why it's failing to import? https://github.com/openedx/brand-openedx/tree/master/paragon/images

@jsnwesson I added that image in cloned brand-openedx (one in png format and one in svg format). I then use that brand-openedx using module.config.js. I imported that image in learner-dashboard as:

import FallbackImageSrc from '@edx/brand/paragon/images/no_course_image.png';

// use it like

<img
        className="pgn__card-image-cap show"
        src={!hasImageError ? bannerImgSrc : FallbackImageSrc}
        alt={formatMessage(messages.bannerAlt)}
        onError={() => {
          if (!hasImageError) { setHasImageError(true); }
        }}
/>

The above works fine and loaded the image. But when I try to use .svg image, I got the error Module not found: Can't resolve 'react' in '/openedx/brand-openedx/paragon/images'. I tried importing svg image like this:

import FallbackImageSrc from '@edx/brand/paragon/images/no_course_image.svg';     OR
import * as FallbackImageSrc from '@edx/brand/paragon/images/no_course_image.svg';

fix: add fallback image for the course card images

0a9b0a7

hinakhadim requested a review from a team as a code owner April 22, 2024 13:54

justinhynes reviewed Apr 23, 2024

View reviewed changes

justinhynes requested changes Apr 23, 2024

View reviewed changes

arbrandes requested changes May 2, 2024

View reviewed changes

deborahgu added the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Jul 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add fallback image for the course card images #324

Add fallback image for the course card images #324

hinakhadim commented Apr 22, 2024

justinhynes Apr 23, 2024 •

edited

Loading

justinhynes Apr 23, 2024 •

edited

Loading

arbrandes May 2, 2024

justinhynes commented Apr 23, 2024

justinhynes left a comment

mphilbrick211 commented Apr 23, 2024

jmakowski1123 commented Apr 24, 2024

hinakhadim commented Apr 25, 2024

justinhynes commented Apr 29, 2024

hinakhadim commented May 2, 2024

arbrandes May 2, 2024

hinakhadim commented May 8, 2024

arbrandes commented May 9, 2024

hinakhadim commented May 10, 2024

arbrandes commented May 15, 2024

hinakhadim commented May 17, 2024

arbrandes commented May 17, 2024

hinakhadim commented May 21, 2024

jsnwesson commented May 21, 2024

hinakhadim commented May 22, 2024 •

edited

Loading

Add fallback image for the course card images #324

Are you sure you want to change the base?

Add fallback image for the course card images #324

Conversation

hinakhadim commented Apr 22, 2024

Before:

After:

Impact:

justinhynes Apr 23, 2024 • edited Loading

Choose a reason for hiding this comment

justinhynes Apr 23, 2024 • edited Loading

Choose a reason for hiding this comment

arbrandes May 2, 2024

Choose a reason for hiding this comment

justinhynes commented Apr 23, 2024

justinhynes left a comment

Choose a reason for hiding this comment

mphilbrick211 commented Apr 23, 2024

jmakowski1123 commented Apr 24, 2024

hinakhadim commented Apr 25, 2024

justinhynes commented Apr 29, 2024

hinakhadim commented May 2, 2024

arbrandes May 2, 2024

Choose a reason for hiding this comment

hinakhadim commented May 8, 2024

arbrandes commented May 9, 2024

hinakhadim commented May 10, 2024

arbrandes commented May 15, 2024

hinakhadim commented May 17, 2024

arbrandes commented May 17, 2024

hinakhadim commented May 21, 2024

jsnwesson commented May 21, 2024

hinakhadim commented May 22, 2024 • edited Loading

justinhynes Apr 23, 2024 •

edited

Loading

justinhynes Apr 23, 2024 •

edited

Loading

hinakhadim commented May 22, 2024 •

edited

Loading