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

Move inline JS + CSS to dedicated files + remove unused pdf_redirect.… #43

Merged
merged 5 commits into from
Jul 10, 2023

Conversation

benoit74
Copy link
Collaborator

@benoit74 benoit74 commented Jul 6, 2023

Rationale

Fix #39

Changes

  • inline javascript of document.html, perseus_exercice.html, epub_embed.html have been moved to dedicated file + cleaned-up from unused variables / code blocks
  • inline CSS of huge <style> block in epub_embed.html has been moved to dedicated file
  • pdf_redirect.html has been removed because it was not used at all in the code base, and contained javascript which was templated so complex to migrate to dedicated file (would probably have needed to adapt the generation, but without the code generating, it was ... complex ^^)
  • add --node-ids CLI parameter to process only few nodes (useful for debugging) with sample command in the README
  • fixed issue with ePub rendering which was outside the iframe

@benoit74
Copy link
Collaborator Author

benoit74 commented Jul 6, 2023

Still a draft for now, I'm performing a scrapper run to confirm it works as intended + checking codefactor result.

@benoit74 benoit74 force-pushed the issue_39 branch 2 times, most recently from 5a49cc3 to b324547 Compare July 6, 2023 06:53
@rgaudin rgaudin marked this pull request as ready for review July 6, 2023 07:23
@rgaudin rgaudin self-requested a review July 6, 2023 07:23
Copy link
Member

@rgaudin rgaudin left a comment

Choose a reason for hiding this comment

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

Changed status of PR to review it. Looks good to me ✅

On pdf_redirect.html, I think it was a first iteration to support PDF by providing access to them (via an HTML redirect).
Eventually, we added PDF.js because of Orange's deployment that lacks a PDF reader (and browser doesn't include it).
I'm not convinced bundled PDF.js is perfect for all cases but we've not used kolibri enough yet. Maybe it will come back as an option at some point. It's fine to remove it now.

@benoit74 please make sure those embed content like epub and pdf work fine with kiwix-serve nightly (on both Firefox and Chrome) as we've had difficulties with iframe sandboxing recently and I'm not sure we've checked kolibri ZIM files.

@benoit74
Copy link
Collaborator Author

benoit74 commented Jul 6, 2023

@rgaudin @mossroy

I'm struggling to reproduce the issue with the ZIM originally proposed in the issue #39 (https://download.kiwix.org/zim/videos/khan-academy-videos_ar_khws-l-dd_2021-12.zim).

From my tests with Kiwix-JS, I do not have any blocking points with Kiwix JS to open this archive (I'm on Kiwix JS 3.9.0 with Firefox 114.0.2 or Chrome 114.0.5735.198 on a Mac).

Regarding epub and pdf, do you have a sample Channel ID with such contents ? I tried to use 878ec2e6f88c5c268b1be6f202833cd4 for Khan Academy (Français) but I do not find any epub or pdf inside.

@rgaudin
Copy link
Member

rgaudin commented Jul 6, 2023

kolibri was originaly developed for libretexts. See https://farm.openzim.org/recipes/libretexts-engineering

@benoit74
Copy link
Collaborator Author

benoit74 commented Jul 6, 2023

There is no (more ?) epub in libretexts-engineering ... any other hint ?

@benoit74
Copy link
Collaborator Author

benoit74 commented Jul 6, 2023

epub are indeed broken, but it looks like it has nothing to do with this change or the ones you did elsewhere.

See below how the scrollbar is at the top but the top of the page is already cropped, and we already see the "bottom" of the epub page.
broken_epub

I have a fix almost ready, I'm currently testing it in a real condition (I hacked a bit to isolate the issue :))

Notice that I also added a commit which introduce a new parameter to process only few nodes.

@benoit74
Copy link
Collaborator Author

benoit74 commented Jul 6, 2023

@rgaudin

I added those two new changes:

- add `--node-ids` CLI parameter to process only few nodes (useful for debugging) with sample command in the README
- fixed issue with ePub rendering which was outside the iframe

I had to rewrite all commits because the first one contained messy console.log debug command in JS file, sorry about that.

Tested with Kiwix JS, pwa.kiwix.org and nightly kiwixserve on MacOS, each time with Firefox and Chrome, looks good so far.

Please review and merge (I do not have sufficient rights, or give them to me ^^)

Btw, I found why I did not achieved to understand the case mentioned in the original issue, it's an archive created by youtube offliner 🙄 🤣

@benoit74 benoit74 requested a review from rgaudin July 6, 2023 13:19
Copy link
Member

@rgaudin rgaudin left a comment

Choose a reason for hiding this comment

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

👍 @benoit74 please push the minor change we did together (move up the option in entrypoint) then merge.

@benoit74 benoit74 merged commit 9798dcd into main Jul 10, 2023
1 check passed
@benoit74 benoit74 deleted the issue_39 branch July 10, 2023 07:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove inline javascript to comply with some CSP
2 participants