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

Better UI for ZIM content: minimal first step #65

Closed
wants to merge 13 commits into from
Closed

Conversation

benoit74
Copy link
Collaborator

@benoit74 benoit74 commented Oct 12, 2023

Fix #53
Fix #31

Changes:

  • Scraper (Python code) has been moved to the scraper subfolder
  • Vue.JS is now used as main UI framework ; all its code is in the zimui subfolder ; it is rendered with Vite to produce a static website
  • QA and Tests workflows have been adapted
    • to the new folder structure
    • to also QA and Test the Vue.JS part
  • precommit hooks have been configured for the Vue.JS part
  • Dockerfile has been adapted to first build the Vue.JS part in a dedicated stage and then embed the generated files into the final Python-based image
  • Topics are stored as JSON files in the ZIM
    • JSON is generated by pydantic
    • these files are consumed by the Vue.JS UI
    • content (video, audio, pdf, epub, ...) is still rendered by Jinja2 as before
  • URLs are meaningful slugs
    • generated by Python slugify lib
    • from Kolibri node title
    • should two distinct nodes have the same title resulting in the same slug, conflicts are handled with a _1, _2, ... suffix
  • changes in the ZIM "folder" structure:
    • files generated by Vite are placed in /
    • thumbnails are placed in /thumbnails
    • JSON files generated to render topics are placed in /topics
    • most Kolibri content (video, audio, ePub, PDF) are placed in /static (some content is still placed at the root to not break some stuff which was found hard to fix for now, will be tackled in specific issues for each content type)
  • legacy MANIFEST.in has been deleted (left-over from migration to hatch)
  • is_front property has been adjusted when adding the item to the ZIM
  • one new CLI argument --zimui-dist to specify the folder where zimui has been built (by Vite)

Known side effects to be discussed:

@benoit74 benoit74 self-assigned this Oct 12, 2023
@kelson42
Copy link
Contributor

@benoit74 @Jaifroid We should verify it works fine with kiwix-js

@benoit74
Copy link
Collaborator Author

Yes about testing it with kiwix-JS, already done by myself of course, plus it is the same base as freecodecamp which was tested AFAIK, but anyway, I will send a file for test once this is ready to be merged

Open questions:

  • Is it possible to configure CodeFactor so that it does not mark the CI as failing when there is an eslint warning? (I had to turn off one rule because there was a warning which is ok from my PoV)
  • Is it ok to merge this into main or should we merge this into a new_ui branch (current branch is already new_ui, so I will have to fix this), so that we can still make fixes to main until we decide to release this big change?

@benoit74 benoit74 marked this pull request as ready for review October 12, 2023 11:46
@benoit74 benoit74 requested a review from rgaudin October 12, 2023 11:46
@rgaudin
Copy link
Member

rgaudin commented Oct 12, 2023

  • Is it possible to configure CodeFactor so that it does not mark the CI as failing when there is an eslint warning? (I had to turn off one rule because there was a warning which is ok from my PoV)

There should be flags for it as for the rest. You need to identify the underlying tool that emits the issue though

Is it ok to merge this into main or should we merge this into a new_ui branch (current branch is already new_ui, so I will have to fix this), so that we can still make fixes to main until we decide to release this big change?

Are there regressions? If no, it should be merged to main IMO

@benoit74
Copy link
Collaborator Author

There should be flags for it as for the rest. You need to identify the underlying tool that emits the issue though

I found them indeed ... eslint-disable-next-line ... will apply it after your review and revert last commit which was disable the rule for all files

Are there regressions? If no, it should be merged to main IMO

There are limitations as mentioned in the first comment, which could be considered like a regression. Somehow my question could be rephrased:

  • do we consider known limitations as regressions which prevent a scraper release?
  • is it ok to release the scraper with only this firs minimal first step in terms of better UI and all other issues still pending?

I would answer yes to both points, but need your PoV as well.

@rgaudin
Copy link
Member

rgaudin commented Oct 12, 2023

  • Scraper (Python code) has been moved to the scraper subfolder

I see no reason to. zimui can be next to the rest of the scraper I think.

  • most Kolibri content (video, audio, ePub, PDF) are placed in /static (some content is still placed at the root to not break some stuff which was found hard to fix for now, will be tackled in specific issues for each content type)

Kolibri channel is mostly files. Those are the content. I think placing them in /static is misleading. We can have a media/ prefix or files/ if you want to stay generic. Feels more appropriate.

@rgaudin
Copy link
Member

rgaudin commented Oct 12, 2023

I found them indeed ... eslint-disable-next-line ... will apply it after your review and revert last commit which was disable the rule for all files

If it's ready you can push it now. Haven't started.

There are limitations as mentioned in the first comment, which could be considered like a regression. Somehow my question could be rephrased:

* do we consider known limitations as regressions which prevent a scraper release?
* is it ok to release the scraper with only this firs minimal first step in terms of better UI and all other issues still pending?

kind of the same question. Please (and that should be standard for UI revamp liek this) share one screenshot so that @kelson42 can weight in.

I don't understand what ”topics are not searchable anymore” mean exactly. The missing breadcrumbs would be terrible with the former UI but maybe that's OK with the new one…

@benoit74
Copy link
Collaborator Author

benoit74 commented Oct 12, 2023

I see no reason to. zimui can be next to the rest of the scraper I think.

This is the layout which has been used for freecodecamp (but maybe this is a bad decision).

@benoit74
Copy link
Collaborator Author

I see no reason to. zimui can be next to the rest of the scraper I think.

And I find it much easier to understand as a dev, that we have a scraper folder which is Python based, and a zimui folder which is Vue.JS based.

@benoit74
Copy link
Collaborator Author

I just fixed the eslint issue + renamed static to files
Regarding the screenshot, this is pretty limited to make a judgment IMHO.
Is it ok if I run Africanstorybook and other test channels locally and upload them to dev library manually?

@rgaudin
Copy link
Member

rgaudin commented Oct 12, 2023

This is the layout which has been used for freecodecamp (but maybe this is a bad decision).

That's exactly why I was surprised to see it here.

@rgaudin
Copy link
Member

rgaudin commented Oct 12, 2023

I see no reason to. zimui can be next to the rest of the scraper I think.

And I find it much easier to understand as a dev, that we have a scraper folder which is Python based, and a zimui folder which is Vue.JS based.

As long as it's thought about and not just a copy of a fringe scraper I'm OK with it.

@rgaudin
Copy link
Member

rgaudin commented Oct 12, 2023

Is it ok if I run Africanstorybook and other test channels locally and upload them to dev library manually?

Sure

@Jaifroid
Copy link

I'd be happy to test a ZIM. I need to test in two modes: ServiceWorker local in a Chromium extension, where inline JS is blocked (and eval, and all eval-like expressions), and ServiceWorker remote, which is the more permissive mode based on (essentially) the remote, offline-first PWA stored at https://browser-extension.kiwix.org.

Freecodecamp works only in the remote mode, because it relies on evaluating strings of JS. Since this is essential to how it works, this is fine. For Kolibri, I imagine it will be expected to work in the former (local-only) mode?

@benoit74
Copy link
Collaborator Author

I forgot about the Freecodecamp usage of eval and its impact on local mode.

I expect Kolibri to work in both modes, and especially in the former (local-only) mode.

@benoit74
Copy link
Collaborator Author

I've just uploaded test ZIMs to dev library (please forgive the filename, I suspect they are not 100% compatible with our naming convention).

@Jaifroid
Copy link

Thanks! I've just tested the "small test" version in Chromium browser extension. The Kolibri UI all seems to work fine. I could access the menus for all resources I tested in the ServiceWorker local mode. (Also tested in PWA mode, of course, no issues).

However, some of the resources contained in the ZIM have eval and inline JS that is blocked in ServiceWorker local mode -- e.g. PheT and the Quizzes, but which works fine in the PWA mode. I realize Kolibri is a container format, and you don't control what is put in the container.

As an aside, I'm not sure we can win the battle against inline JS and eval given that ZIM archives are increasingly becoming containers for content from a very wide range of web resources over which we don't have full control.

But thanks for making the UI conformant! 👍

@benoit74
Copy link
Collaborator Author

Thank you for the test!
And yes, Kolibri is a container.
In some cases there are only PDF / ePub documents so we are "safe", but in other situation we have more varied kind of content which are more ... varied 😆

@benoit74
Copy link
Collaborator Author

As discussed Friday, the search for a topic must be fixed and this cannot be merged to main until two known limitations are fixed, so I will close this PR and reopen another one targeting a temporary branch.

@benoit74 benoit74 closed this Oct 16, 2023
@benoit74 benoit74 mentioned this pull request Oct 16, 2023
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.

Better UI: minimal first step URLs should be meaningful
4 participants