fix: course image height on IOS Safari

[Cup0fCoffee]

Description

Closes #265.

Course thumbnails on IOS Safari stretch to the full height of the image, instead of being limited by width and preserving aspect ratio. This seems to be a IOS Safari specific behavior[1], and can be fixed by setting height to auto, while width is set to 100%.

This issues is specific to the context of the course card, so we are fixing it by setting the height in the course card css, instead of fixing it globally in the Paragon styles, which wouldn't work in all contexts.

1: https://stackoverflow.com/a/44250830

Screenshots

Screenshots taken from an iPhone of the same course on the course dashboard before and after the fix. Course name censored for privacy.

Before

After

[openedx-webhooks]

Thanks for the pull request, @Cup0fCoffee!

This repository is currently maintained by @2U-aperture.

Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review.

🔘 Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

• If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
  • This process (including the steps you'll need to take) is documented here.
• If it doesn't, simply proceed with the next step.

🔘 Provide context

To help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:

• Dependencies

  This PR must be merged before / after / at the same time as ...

• Blockers

  This PR is waiting for OEP-1234 to be accepted.

• Timeline information

  This PR must be merged by XX date because ...

• Partner information

  This is for a course on edx.org.

• Supporting documentation
• Relevant Open edX discussion forum threads

🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

---

Where can I find more information?

If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:

• Overview of Review Process for Community Contributions
• Pull Request Status Guide
• Making changes to your pull request

When can I expect my changes to be merged?

Our goal is to get community contributions seen and reviewed as efficiently as possible.

However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:

• The size and impact of the changes that it introduces
• The need for product review
• Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.36%. Comparing base (9228d01) to head (46b7929).
Report is 2 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #553   +/-   ##
=======================================
  Coverage   97.36%   97.36%           
=======================================
  Files         155      155           
  Lines        1365     1365           
  Branches      229      229           
=======================================
  Hits         1329     1329           
  Misses         34       34           
  Partials        2        2           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[jsnwesson]

@Cup0fCoffee I'm hesitant to bring in more SCSS overrides in this repo, especially when it originates in Paragon and ideally the fix could be made there. However, I know that Learner Dashboard is stuck on v22 due to its dependency on frontend-platform, which hasn't been updated to v23. I guess what I'm trying to say is, could this fix exist in Paragon, so that whenever we finally update to v23 Learner Dashboard inherits that fix? With that, I'd worry that this override would be forgotten here once that version update takes place.

[Cup0fCoffee]

@jsnwesson My initial thought process was that this fix is only applicable in this context, and adding it to Paragon would mess up other cases where these styles are used, so adding it as a custom rule made sense to me. I thought this answer is good enough, but they I had another look, and reconsidered.

I found three issues and one closed PR related to the same issue:

• [bug]: Card component stretches image on Safari paragon#3187
• discovery: learner dashboard use case for Card.ImageCap paragon#2110
• Course image height #265
• style(CourseCardImage.jsx): img width change to w-100 for safari browser #461

From them I learned the following things:

• This issue is present in both - the Paragon (Card component) and this MFE.
• This MFE doesn't use Card.ImageCap provided by Paragon. It is because Paragon's implementation didn't allow image or title to be clickable, but it might have been fixed since then.
• I forgot that some styles can be applied via classes, e.g. w-100 or h-auto.

---

So to answer your question:

could this fix exist in Paragon, so that whenever we finally update to v23 Learner Dashboard inherits that fix?

To fix this issue through Paragon, it would be necessary to:

1. Find and test a solution that fixes this problem and doesn't break any other places where Card with an image it used.
2. Change the implementation of Card in Paragon so that this MFE doesn't need to use custom implementation.
3. Upgrade this MFE to the new version of Paragon.
4. Migrate this repo's implementation of Card to Paragon's.

I don't see an easy solution for #1 and the rest could take quite some time.

However, as a compromise, we could keep this fix local for now, to fix this issue early, and at the same time, instead of using custom styles tucked away in a SCSS file where they could get forgotten, we could add a w-100 or h-auto class here and add a comment that would provide the context for it.

What do you think about that? Does that address your concerns?

[jsnwesson]

@Cup0fCoffee phew, thanks for doing the digging! I completely agree that the time to make the fix in Paragon could take time, and I know there's already a blocker being experienced with getting the MFEs to v23 anyways, so the alternative fix you suggest sounds like a great compromise.
One thing to say about your first point around testing the affect on other MFEs implementing the Card, I'd imagine it'd be far more work to track down every MFEs' Card implementation, and instead this change would just need to be properly flagged as a change to look out for when the MFEs upgrade to v23. I could be wrong on that assumption, but just figured it was worth mentioning in case you want to submit a PR to Paragon in the future to fix it :D

[Cup0fCoffee]

@jsnwesson Great, I will change the implementation as soon as the client approves additional budget for the change and testing :)

[Cup0fCoffee]

@jsnwesson I've updated the PR.

[jsnwesson]

thanks for this @Cup0fCoffee !