Skip to content
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

Add a thumbnail component for new hybrid learning features #8225

Merged
merged 2 commits into from Aug 3, 2021

Conversation

MisRob
Copy link
Member

@MisRob MisRob commented Jul 27, 2021

Summary

For my work on the completion modal #8109, I needed to create a new thumbnail component which also draws from recent JB's hybrid learning work. I'll soon be preparing the rest of JB's work for merge and I will need to have this component available. Because we won't be able to merge the completion modal for some time, I've extracted the thumbnail logic from the completion modal to this separate PR.

The component displays a topic or learning activity icon when a picture is not available. Regarding the thumbnail container ratio, we decided to go with 16:9 as that's the most common ratio of thumbnails. Pictures with different ratios are letterboxed. The thumbnail adjusts its width according to its container width to allow for flexible control from parent components which is important when implementing responsive behavior across hybrid learning features.

Besides that, this PR extends the utility function for getting thumbnails from the content node to account for cases when simplified data are returned from API for a content node where the thumbnail is saved directly in the thumbnail attribute.

Thumbnail component screenshots

16:9 thumbnail-16:9
4:3 thumbnail-4:3
1:1 thumbnail-1:1
wide thumbnail-wide
no thumbnail - single learning activity no-thumbnail-watch
no thumbnail - multiple learning activities no-thumbnail-multiple_activities
no thumbnail - topic no-thumbnail-topic

References

As for the decisions about icons colors and thumbnail container ratio, you can see this Slack thread.

Reviewer guidance

Since this is only a prerequisite component for work that will be reviewed in the future, I think that briefly checking code and screenshots is sufficient at this point. However, if you'd like to preview it locally on the dev server, I can provide my completion modal working branch.

Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical user journeys are covered by Gherkin stories
  • Critical and brittle code paths are covered by unit tests

PR process

  • PR has the correct target branch and milestone
  • PR has 'needs review' or 'work-in-progress' label
  • If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
  • If this is an important user-facing change, PR or related issue has a 'changelog' label
  • If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

@MisRob MisRob added the TODO: needs review Waiting for review label Jul 27, 2021
@indirectlylit indirectlylit changed the base branch from develop to release-v0.15.x July 27, 2021 18:20
@indirectlylit indirectlylit added this to the 0.15.0 milestone Jul 27, 2021
@MisRob
Copy link
Member Author

MisRob commented Jul 31, 2021

@indirectlylit What was the reason for retargeting this PR to release-v0.15.x? I will need it in develop for the completion modal and other hybrid learning features that can't go to release-v0.15.x. I saw there are some directions for targeting develop for all hybrid learning work or are there any changes to those decisions?

@indirectlylit
Copy link
Contributor

Good catch – this should be in develop for hybrid learning and doesn't need to be in the current 0.15 release branch.

By the way, sorry I haven't had a chance to review this yet. It looks really valuable, I'm excited to take a look!

@indirectlylit indirectlylit changed the base branch from release-v0.15.x to develop July 31, 2021 14:04
@MisRob
Copy link
Member Author

MisRob commented Jul 31, 2021

Thank you. No worries, I'd post a message to someone if it'd be urgent.

Copy link
Member

@marcellamaki marcellamaki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me @MisRob! I've only done a read-through and reviewed the screenshots, but the code makes sense and I agree that any implementation issues will become apparent as this component is used in features that we are working on.

@rtibbles rtibbles merged commit 249b93b into learningequality:develop Aug 3, 2021
@MisRob MisRob deleted the thumbnail branch September 5, 2021 10:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
TODO: needs review Waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants