Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Clean up renderer height styling behaviour #11383

Conversation

rtibbles
Copy link
Member

@rtibbles rtibbles commented Oct 10, 2023

Summary

  • Removes use of Learn specific height remediations from HTML5 app renderer and PDF renderer
  • Reworks styling of PDF renderer to no longer require a global attribute setting on the page html tag, which was causing an issue when the PDF renderer was embedded in pages that needed to scroll independently of the PDF
  • Makes styling of the PDF renderer consistent as long as the embedded use of the KContentRenderer has a specified height

References

Fixes #11311

Reviewer guidance

Create a lesson
Manage resources
Try to add a PDF and preview it
Observe that the page can still scroll
image

View a PDF in Learn, ensure that it behaves as expected
image

View an HTML5 app in Learn ensure that it behaves as expected

image


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

@rtibbles rtibbles added the TODO: needs review Waiting for review label Oct 10, 2023
@github-actions github-actions bot added APP: Learn Re: Learn App (content, quizzes, lessons, etc.) APP: Coach Re: Coach App (lessons, quizzes, groups, reports, etc.) SIZE: small labels Oct 10, 2023
Copy link
Member

@nucleogenesis nucleogenesis left a comment

Choose a reason for hiding this comment

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

Looking through the code things look good. I'll spin it tomorrow so I can better visualize the styles in devtools and test things out.

Copy link
Member

@pcenov pcenov left a comment

Choose a reason for hiding this comment

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

Both pdf and HTML resources are displayed correctly except in mobile view where the PDFs are initially displayed in all black that is until the user selects the bookmarks menu. Seems that this is happening when viewing in resolution less than 840 px.

2023-10-11_15-19-14.mp4

@rtibbles
Copy link
Member Author

Interesting, thanks for the check, @pcenov! I'll see what's happening there.

Rescale on first load if sidebar is shown.
@rtibbles
Copy link
Member Author

OK, I think this should all be working as expected now!

@pcenov pcenov self-requested a review October 12, 2023 09:26
Copy link
Member

@pcenov pcenov left a comment

Choose a reason for hiding this comment

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

I confirm - everything is working as expected now.

@rtibbles rtibbles merged commit 0fbcc1a into learningequality:release-v0.16.x Oct 12, 2023
@rtibbles rtibbles deleted the all_renderers_great_and_small branch October 12, 2023 16:34
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
APP: Coach Re: Coach App (lessons, quizzes, groups, reports, etc.) APP: Learn Re: Learn App (content, quizzes, lessons, etc.) SIZE: medium SIZE: small TODO: needs review Waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Coach>Plan/Reports - Missing scrollbar when previewing a resource of type PDF
3 participants