-
-
Notifications
You must be signed in to change notification settings - Fork 12
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
New UI step 1 #68
Conversation
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 |
|
|
Ah, OK, I hadn't associated that comment with this error. Thanks. |
@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:
|
There was a problem hiding this 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.
e662bed
to
c6a22f6
Compare
c6a22f6
to
96440af
Compare
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). |
There was a problem hiding this 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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@benoit74 I have copied the two tickets fixed by previous version of this PR in the description. Hope this is OK. |
@benoit74 I have finally check the new ZIM, here my remarks:
|
No it's not, see my last comment #68 (comment)
Thank you. And yes, this is the very beginning, this is why we have so many new issues to fix, all titled "Better UI"
Will be covered by #55 ; same for all other kind of assets in fact
Idem, this is the old design
Will be covered by #67
Idem, they are working if you search for a PDF / ePub title AFAIK
This is the old design
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.
This is the original Endless design
There is no issue (need ?) for this so far AFAIK
I don't get what you are speaking about |
@rgaudin I think I've fixed your last comment, let merge if this is ok for you. |
It is, thank you |
This is a reopen of #65 with an intermediate base branch
new_ui
since know limitation below prevent us to merge tomain
Changes:
scraper
subfolderzimui
subfolder ; it is rendered with Vite to produce a static websitepydantic
slugify
lib_1
,_2
, ... suffix/
/thumbnails
/topics
/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)is_front
property has been adjusted when adding the item to the ZIM--zimui-dist
to specify the folder where zimui has been built (by Vite)Known side effects to be discussed: