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

New UI step 1 #68

Merged
merged 21 commits into from
Oct 23, 2023
Merged

New UI step 1 #68

merged 21 commits into from
Oct 23, 2023

Conversation

benoit74
Copy link
Collaborator

@benoit74 benoit74 commented Oct 16, 2023

This is a reopen of #65 with an intermediate base branch new_ui since know limitation below prevent us to merge tomain

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 /files (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 16, 2023
@benoit74 benoit74 marked this pull request as ready for review October 16, 2023 08:39
@benoit74 benoit74 requested a review from rgaudin October 16, 2023 08:39
@Jaifroid
Copy link

I realize this relates to the previous PR, but I suppose it will carry over to this one. I noticed that there are errors in the quizzes contained in small_test_new_ui_2023-10.zim. I wondered whether you also see these in other apps, or whether it might be an issue with the Kiwix JS software. See screenshot below. The quiz still works, so it's only cosmetic, but it's unusual that missing stylesheets can be displayed as errors in the UI:

image

@rgaudin
Copy link
Member

rgaudin commented Oct 16, 2023

  * 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)

#65 (comment)

@rgaudin
Copy link
Member

rgaudin commented Oct 16, 2023

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

  * 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)

#65 (comment)

Ah, OK, I hadn't associated that comment with this error. Thanks.

@rgaudin
Copy link
Member

rgaudin commented Oct 16, 2023

in ASB, frequently (not systematically), when I browse one language and come back, I find myself at the bottom of the page (seeing Yoruba), while I left homepage on top of the page. This is disturbing.

I find those arrows strange ; are those from the Endless UI? Can you share the endless UI link again? Would help compare.
Screenshot_2023-10-16_at_15_29_09

assets/perseus/index.html is apparently is_front since it's served by random()

When I was testing I encountered many times a broken image on the first page of PDF (the thumbnail) and then it worked and I can't reproduce. In case you're seen it as well, there might be an issue somewhere ; if you did not, we'll have to wait for it to resurface.

@benoit74
Copy link
Collaborator Author

@Jaifroid thank you for this report ; it is not normal and will be fixed. Maybe this is displayed in the page due to perseus codebase which handle loading these assets very nicely? (I don't know tbh)

@rgaudin thank you for these first feedbacks:

  • I did not encountered the broken image on the first page of the PDF, I will check on a browser with a cleared cache
  • I will fix as well the "bottom of the page" issue
  • Your screen is definitely too big, I will buy you a smaller one 🤣 ; I knew the arrows where not perfect, but they are definitely bad on such a big screen ; I will fix this as well ; original endless UI (and link to it) is in the issue Better UI: minimal first step #53

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.

Thank you. Can you also update the changelog? It's part of our process to update it along changes.

.gitignore Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
scraper/pyproject.toml Show resolved Hide resolved
scraper/src/kolibri2zim/schemas.py Outdated Show resolved Hide resolved
scraper/src/kolibri2zim/schemas.py Show resolved Hide resolved
zimui/src/components/TopicSection.vue Outdated Show resolved Hide resolved
zimui/src/components/TopicSection.vue Outdated Show resolved Hide resolved
zimui/src/components/TopicSection.vue Show resolved Hide resolved
zimui/src/components/TopicSection.vue Show resolved Hide resolved
zimui/src/components/TopicCard.vue Show resolved Hide resolved
@benoit74
Copy link
Collaborator Author

I think I've sorted out all comments.

I've added issues I consider mandatory before merging this base branch to the 1.2.0 milestone. Feel free to speak up if there is something missing (or something not mandatory).

@benoit74 benoit74 requested a review from rgaudin October 20, 2023 12:55
@benoit74
Copy link
Collaborator Author

And I've removed the "Fix #xxx" so that issue #53 is not closed until all other issues are closed, and #31 is not closed until decision is made regarding slugs computation.

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.

Thank you ; I believe it's good to go 👍

The CamelCase change is unexpected in-between reviews. Please mention such changes in your comments next time.

Can you also open a ticket on your repo of choice to implement get_web_deps in Python?

this.channelData = response.data as Channel
},
(_) => {
this.isLoading = false
Copy link
Member

Choose a reason for hiding this comment

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

Maybe log the error in the console?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This will be tackled more appropriately by #70

return response.data as Topic
},
(_) => {
this.isLoading = false
Copy link
Member

Choose a reason for hiding this comment

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

Same

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Same also 😀

const limitCardsPerSections = (inputArray: any[], section_slug: string) => {
/**
* Keep only the 10 first items of the input array. If more than 10 items
* where present in the list, a "magic/virtual" 11th item is created to indicate
Copy link
Member

Choose a reason for hiding this comment

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

typo “were”

): TopicCardData[][] => {
return cards.reduce(
(all: TopicCardData[][], one: TopicCardData, i: number) => {
const ch = Math.floor(i / cardsPerChunk)
Copy link
Member

Choose a reason for hiding this comment

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

I think we can afford words for variable name: chunk, index. Even all is vague

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@kelson42
Copy link
Contributor

@benoit74 I have copied the two tickets fixed by previous version of this PR in the description. Hope this is OK.

@kelson42
Copy link
Contributor

@benoit74 I have finally check the new ZIM, here my remarks:

  • We have a clear improvement, even if I believe this is still the very beginning.
  • EPUB filename should be meaningfull. For example qaxalefi-obboleewwan-isaa.epub at https://dev.library.kiwix.org/viewer#africanstorybook.org_mul_all_newui_2023-10/static/qaxalefi-obboleewwan-isaa
  • I don't really understand the role of the one tab "Main document (EPUB)". It does not make sense to have a tab system if there only one tab + "Main document (EPUB)" does not bring anything useful for the user. I would simply remove things and just keep the button to download the EPUB. I kind of suspect that we might have situation where we have many tabs... but here this is not the case in the whole ZIM.
  • Random button seems broken and returns alwasy https://dev.library.kiwix.org/viewer#africanstorybook.org_mul_all_newui_2023-10/assets/perseus/index.html. I believe the random (and the suggestions) should work only on the stories and categories.
  • Suggestions not working at all looks like.
  • The interesting part of a page with a story is the book (EPUB renderer), but the book itself is pretty small an takes only a very small portion of the screen (with big margins everywhere). I suspect the book should be displayed bigger.
  • We definitely have a problem with everywhere the same thumbnail for categories
  • I wonder if the button "OPEN" on thumbnail makes sense at all, but I question its behaviour if the whole content is in French. Localisation (even if still not existing) should be taken in consideration.
  • Similar as for youtube, we should find a way - automatically or not - to get ride of this basis white/grey background colors.

@benoit74
Copy link
Collaborator Author

benoit74 commented Oct 22, 2023

@benoit74 I have copied the two tickets fixed by previous version of this PR in the description. Hope this is OK.

No it's not, see my last comment #68 (comment)

We have a clear improvement, even if I believe this is still the very beginning.

Thank you. And yes, this is the very beginning, this is why we have so many new issues to fix, all titled "Better UI"

EPUB filename should be meaningfull. For example qaxalefi-obboleewwan-isaa.epub at https://dev.library.kiwix.org/viewer#africanstorybook.org_mul_all_newui_2023-10/static/qaxalefi-obboleewwan-isaa

Will be covered by #55 ; same for all other kind of assets in fact

I don't really understand the role of the one tab "Main document (EPUB)". It does not make sense to have a tab system if there only one tab + "Main document (EPUB)" does not bring anything useful for the user. I would simply remove things and just keep the button to download the EPUB. I kind of suspect that we might have situation where we have many tabs... but here this is not the case in the whole ZIM.

Idem, this is the old design

Random button seems broken and returns alwasy https://dev.library.kiwix.org/viewer#africanstorybook.org_mul_all_newui_2023-10/assets/perseus/index.html. I believe the random (and the suggestions) should work only on the stories and categories.

Will be covered by #67

Suggestions not working at all looks like.

Idem, they are working if you search for a PDF / ePub title AFAIK

The interesting part of a page with a story is the book (EPUB renderer), but the book itself is pretty small an takes only a very small portion of the screen (with big margins everywhere). I suspect the book should be displayed bigger.

This is the old design

We definitely have a problem with everywhere the same thumbnail for categories

This is why I am asking for a rework of Kolibri channel for African Storybook, once this is fixed we will have proper thumbnails. These thumbnails are supposed to be here and are handled if present, I don't want to create them on the scraper side.

I wonder if the button "OPEN" on thumbnail makes sense at all, but I question its behaviour if the whole content is in French.

This is the original Endless design

Localisation (even if still not existing) should be taken in consideration.

There is no issue (need ?) for this so far AFAIK

Similar as for youtube, we should find a way - automatically or not - to get ride of this basis white/grey background colors.

I don't get what you are speaking about

@benoit74
Copy link
Collaborator Author

@rgaudin I think I've fixed your last comment, let merge if this is ok for you.

@rgaudin
Copy link
Member

rgaudin commented Oct 22, 2023

@rgaudin I think I've fixed your last comment, let merge if this is ok for you.

It is, thank you

@benoit74 benoit74 merged commit de74cee into new_ui Oct 23, 2023
6 checks passed
@benoit74 benoit74 deleted the new_ui_step_1 branch October 23, 2023 06:09
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.

4 participants