Hi Zoran, I just pushed some more fixes, I hope this isn’t making reviewing too hard.
Starting far back in the email chain is probably pretty confusing, since I or others have fixed a lot of the problems noted. I’ve tried to summarize all the current problems in each email: I think I should have also noted which ones got fixed. Some comments inline. > On Sep 7, 2021, at 4:42 AM, Zoran Regvart <zo...@regvart.com> wrote: > > Hi David, > working through the changes and emails, some more comments here... > > On Fri, Sep 3, 2021 at 8:21 AM David Jencks <david.a.jen...@gmail.com> wrote: >> comments: >> >> - I removed the patch-sitemap.js use. I can’t figure out why this is >> needed. If it really is needed, it should be done with a pipeline extension. > > I think we'll need to restore this, otherwise we'll lose the sitemap > generation which in turn leaves us with outdated links on the search > engines, and as we change the links when we add or remove new > documentation versions this becomes an issue. > I’m rather confused by this since my experience and Dan’s repeated statements are that Antora always generates relative links within the site. It’s possible to write the UI to generate either absolute or relative links, but that’s not something Antora is doing itself. Getting Antora to generate absolute links in page bodies (not the UI) requires major surgery :-) So, I’ll compare generating with absolute and ‘/‘ siteUrl and come back with more definite info. >> - yarn workspace —topological-dev is not building the UI before it’s used. >> I can’t figure out why; it is for other PRs. I replaced this by just >> calling the UI build directly. > > That's odd, I did not notice this issue before, I'll try to reproduce > it and see if there is something we can do about this, this should be > one of the benefits in using workspaces. It doesn’t seem to afflict other people’s PRs :-), but I have not yet had any build use the correct order with —topological-dev. One run showing the problem is https://github.com/apache/camel-website/runs/3453859096?check_suite_focus=true. I wonder if it’s related to checking in the wrong .yarn contents: I think in this particular run I hadn’t checked in any .yarn changes, but later I have been checking in such changes and still get the wrong order. > >> - If nothing else is using the model building in UpdateReadmeMojo it can >> probably be simplified further. > > From what I understand we only need the JSON files generated now, > perhaps the we don't need the UpdateReadmeMojo at all any more? Perhaps. It still generates the doc header. If Allow asciidoctor extensions when processing nav files and headers <https://gitlab.com/antora/antora/-/issues/592> were implemented, the header generation could be done by the jsonpath extension, but I’m not sure how much of an improvement that would be. In any case, UpdateReadmeMojo is now more of a sanity checker than content generator :-). > >> - The camel-website node dependencies have changed dramatically. It looks >> like the yarn cache is checked into git; I haven’t checked in the changes. >> Should I? > Later on I started checking in .yarn, but I’m really not sure if I’m getting appropriate content. I think that previously the UI was not in the main .yarn, but at least sometimes I get it, and it’s > 50 MB and I get a warning. Note that I have 2 camel-website PRs: the “antora 3” one is the one that should be merged, and after the camel jsonpath-options PR is merged it will build with all the new stuff. The other camel-website PR is just to build the jsonpath-options branch. > >> known problems: >> >> - When the first row contains a description with a list, the list isn’t >> rendered properly. cf. >> https://pr-614--camel.netlify.app/components/latest/activemq-component.html#_path_parameters_2_parameters >> >> <https://pr-614--camel.netlify.app/components/latest/activemq-component.html#_path_parameters_2_parameters> > > I guess this is fixed now, looks okay to me... Yes, this is one of the many problems now fixed :-) > >> - All the names in the generated tables have ids so they can be linked to >> but there’s no easy way to find out the id. The sections have a link to >> themselves which makes it easy to copy the URL. I haven’t looked into >> whether this can be done with the table entries. > > I think this is a nice to have, so we can definitely merge this > without this and followup after... > >> Updating the main PR to track changes in camel is somewhat time consuming so >> I’d appreciate it if we could move this towards resolution quickly. > > Yes, I'm looking into this right now, my goal is to get this merged _very_ > soon! Thanks for the review!! David Jencks > > zoran > -- > Zoran Regvart
