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

Add breadcrumbs back + fix meaningfull URLs + add developpers instructions #74

Merged
merged 5 commits into from
Nov 2, 2023

Conversation

benoit74
Copy link
Collaborator

@benoit74 benoit74 commented Oct 27, 2023

Would solve #66 and #64 and #31 once merged to main, but not yet ready to be merged in this branch (other issues have to be fixed)

Changes:

  • removed outdated release instructions (we now have a Github task issue)
  • developer instructions for local Vue.JS debugging
  • add breadcrumbs back
  • slug value is always computed where requesting a node to database
    • slug is populated in node, its parents and children
    • this has the side effect that node parents and children are not generators anymore
  • final computation of node slug as agreed on issue URLs should be meaningful #31
    • in case of slug conflict for two nodes, scraper stops

@benoit74 benoit74 force-pushed the new_ui_step_2 branch 2 times, most recently from 9c342f8 to fe3ecc1 Compare October 27, 2023 13:36
@benoit74 benoit74 changed the title WIP: Add developper instructions for ZIM UI based on Vue.JS Add breadcrumbs back + add developpers instructions Oct 30, 2023
@benoit74 benoit74 marked this pull request as ready for review October 30, 2023 15:10
@benoit74 benoit74 marked this pull request as draft October 30, 2023 15:14
@benoit74 benoit74 changed the title Add breadcrumbs back + add developpers instructions Add breadcrumbs back + fix meaningfull URLs + add developpers instructions Oct 30, 2023
@benoit74 benoit74 marked this pull request as ready for review October 30, 2023 15:47
@benoit74 benoit74 requested a review from rgaudin October 30, 2023 15:47
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 ;

Have you had the chance to run it on a large channel like Khan ?

CONTRIBUTING.md Outdated Show resolved Hide resolved
scraper/src/kolibri2zim/scraper.py Show resolved Hide resolved
@benoit74
Copy link
Collaborator Author

benoit74 commented Oct 31, 2023

Have you had the chance to run it on a large channel like Khan ?

Nope ... Is there a language which would make more sense? en/es/fr?

@rgaudin
Copy link
Member

rgaudin commented Oct 31, 2023

I'd say FR because it's large and easier for you to spot issues when browsing

@benoit74
Copy link
Collaborator Author

benoit74 commented Nov 1, 2023

I just started Khan Academy FR on my machine, it will probably run for many days so I propose to move forward this PR without waiting for the result, we are not yet at the point of merging this to main anyway.

@benoit74 benoit74 requested a review from rgaudin November 1, 2023 08:09
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
Copy link
Collaborator Author

benoit74 commented Nov 2, 2023

@rgaudin I assume it means I can resolve the conversation and merge the PR

@benoit74 benoit74 merged commit 8b29fab into new_ui Nov 2, 2023
6 checks passed
@benoit74 benoit74 deleted the new_ui_step_2 branch November 2, 2023 08:51
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.

2 participants