-
Notifications
You must be signed in to change notification settings - Fork 0
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
Initial JS UI #33
Initial JS UI #33
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #33 +/- ##
==========================================
- Coverage 92.72% 91.66% -1.07%
==========================================
Files 4 4
Lines 330 348 +18
Branches 44 42 -2
==========================================
+ Hits 306 319 +13
- Misses 22 25 +3
- Partials 2 4 +2 ☔ View full report in Codecov by Sentry. |
b05ae91
to
6f757db
Compare
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.
Awesome!
I'm really puzzled by the fact that you still render one HTML page per resource, and more or less dynamically hook Vue.JS css and js code into this. It looks a bit like a hack from my PoV, what is the advantage? Is this just an interim situation until everything is moved to Vue.JS?
It is also weird to see the navbar disappear and reappear when opening another page, and collapsing entries which were uncolapsed. This is purely a side-effect of reloading the Vue.JS app.
I also feel like it would simplify the setup to move page content to JSON files as well, and load the full Vue.JS app (including index.html) for rendering the whole page. It would need some more work to properly index things for suggestions and full-text search, but nothing dramatic to implement.
And finally, I still achieve to select two entries at once since we rely on its href to know which navbar entry has been selected, and we do have duplicates (see e.g. in cpp, in "Input/output" we have "egptr" and "gptr" entries both pointing to same href, so clicking any of them highlights both entries.
Very good work anyway, but some rough edges left to polish ^^
It was the best way I could come up with to do this within the constraints we have (without opening another can of worms), very open to better options. :-) DevDocs tries to keep the structure of the URLs from the original so page to page links are relative. I think that means we need to do one of the following:
I'm worried about introducing bugs if we start rewriting content or messing with the page structure too much because DevDocs' CSS is...very tailored to their exact approach. Especially if we want to take advantage of their dark mode and per-language style normalization overrides. Is there an obvious solution I might be missing?
An alternative approach I had been thinking about would be something similar to Python's docs with a big index and the navbar only showing the current section's links: https://docs.python.org/3/reference/lexical_analysis.html#line-structure. That could split the difference between dynamic and static content.
I think this means we'll also have to rewrite doc content to fix links because we won't be able to take advantage of a service worker to dynamically return content in the same way that DevDocs does.
Sure, I think we can work that case out to some degree once we settle on the final approach. |
Of course, the thing I missed was this damn relative links which will get broken if we mess with it. FYI, we have already working code to rewrite HTML statically (i.e. at scrape time, in Python) and dynamically (i.e. at rendering time, with a JS library - wombat - capable to intercept any web request before it is issued so and rewrite the target URL to be "in-zim"). They are both used for zimit/warc2zim, but I'm close to move them to python-scraperlib because I need them for a new scraper I'm working on. The HTML static rewriting engine is quite powerful in the sense it is easily customizable to your specific needs. I think they could work as well for devdocs. But you are totally right that this is more work, and could be a bit tricky to setup, where your approach is probably a bit more solid in terms of links, but a more fragile / limited in terms of UI / UX and risk of breaking something. Since the best is the enemy of the good, I think it is worth to continue with this PR approach, and keep in mind we might want to change this at some point. WDYT? |
This sounds great! There's also an open issue here to bring in external resources like images, I think that'll be a perfect time to adopt rewriting the links too because we'll need to rewrite the
That works for me, I'll get these remaining questions closed out and send it back for review. As always, thank you for the quick turnarounds! |
4ba5dc3
to
c751032
Compare
Alright, I think all the requested changes have been made. And we've now resolved the highlighting issue (somewhat) with multiple links that point to the same page -- only one highlights and clicking on others won't reload the page but if you're navigating from a different page the original link you picked won't be preserved. |
c751032
to
00f914e
Compare
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.
Almost OK, one very small change needed and one suggested.
00f914e
to
8b59618
Compare
8b59618
to
94378e6
Compare
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.
LGTM
Initial JavaScript UI for the ZIMs. This dramatically reduces the per-page size for large DevDocs and the time needed to render them.
In order to keep search working well the approach is to create a custom element that renders the navbar from each page rather than using an SPA. The custom element loads the navbar data from a JSON structure that's based on the previous Jinja template objects.
Ansible which has ~6500 pages is only 15M and only takes about a minute to build:
CPP, which was previously 260MB is now only 7MB.
Fixes #24, #31