docs(general): improve sidebar behaviour for docs#550
Conversation
|
The sidebar is so so so much better omg The changes to the page look good too, we should definitely pick names like description and notes or intro and outro or something though instead of pre and post. The problem with the changes to the page I am having is we are starting to stray quite far from commonmark and into mdbook specific territory. I want to turn the docgen into something that can generate markdown for 3rd party modules, if we get too mdbook-specific there, that starts to be a problem for people who want to include it in a site made with a different markdown renderer. Using If you can achieve a still acceptable result with a bit less html formatting on the markdown side, that would be appreciated. But it is way way better. Thank you. The difference with the sidebar was more dramatic than I expected. I expected it to be just a bit better, but it was a lot better instead. |
|
I see, that makes sense. I will look into how we can get something close without the added html. As a separate idea, would adding a post-processing step to the build be something you are okay with? I was thinking it could be a script that runs through all the module html files and adds what we want once the book is built. That way the docgen will still produce pure markdown, and then anything specifically for the nix-wrapper website can be added later. |
Im not convinced adding another post processing step beyond what mdbook allows us to do is better than just generating it correctly? I would rather add a little html to the markdown than post-process the html, and maybe someone figures out how to remove more later. Plus if the generated docs are not somewhat close to ours, people won't want to use it anyway. Just do the minimum you can to the markdown in terms of html stuff, (but prefer html to handelbars), and that should be good enough, and we can work on anything left later when people complain about it not working with their generator haha. Best effort is enough on that for now, especially because right now it is somewhat single purpose and the docs being better is somewhat more important than these future concerns. I just wanted you to be aware of that as something to think about before you added more in that direction. |
|
Sounds good. Another idea I had was we could add a flag to the docgen to include the html flavour? Or maybe a param which takes the engine it should generate for. Then we could make some of the more complex stuff optional based on that. |
|
We could yeah, we could have it import the mdbook module with an extra module which adds the stuff for the sidebar. But lets handle that when we get to it. Ideally, we don't have to modify the markdown for that purpose, but if we have to then we have to I guess. |
|
I was able to remove everything except for the details/summary blocks. The one side effect of using md for the headings inside the summary blocks (instead of the h3 tags) is that they now show up in the side bar. This actually feels better to me except for maybe the 'Options' header. I tried nesting the options themselves under the header, but setting them to use a heading at level 4 made them too small on the page and added too many levels to the sidebar in my opinion. Therefore, this feels like the best compromise. If you don't like it, we could try setting it as plaintext inside the summary element and then making it larger and bold through css. That way it looks like a heading on the page (without using html) but isn't picked up by the sidebar. |
So, it actually does not. You did better than that. It relies on the fact that module headings are the only level 2 headings INSIDE the That should be plenty good enough. Not only that, but these things are all things directly in our control, not mdbook. All the generators are supposed to make ### an h3 and details a details if they support the details thing, which almost all of them do. I think this is reliable enough. I am OK with them showing up in the sidebar if it means we can emit more standard markdown and have less to maintain. It is not that important to me the exact presentation, the information just needs to be presentable enough that someone can navigate it and see what a module offers, which, if we splatted 8000 lines of markdown converted to html for every module in alphabetical order onto the page, it would not reach that bar. Which is why I insisted that they be collapsible. And now you have achieved that for the sidebar too. This is good. Someone could use this generator to generate markdown to use directly on github, and it would also be able to render this. I think that is a fine bar to pass, and this passes it. I will need to edit the pre and post description in the neovim module to not have such aggressive markdown headings, but that is not a problem with your generator, just my markdown 😸 Edit: my neovim markdown previewer cant quiiiiiite do it, but it is more than close enough. Github can, I just copied the dir into a random dir with Now I need to fix the collection logic, and then I can start thinking about what the user interface of these doc options is to look like! |
|
Great, I am glad to hear that! 3 final questions:
|
I did start taking a look at this, but I didn't get very far. I think I will have time to do more in about 2 weeks. If you don't have time or don't want to look at it before then, I am happy to take it on. |
|
If you want to. I am not exaggerating when I said it is a hard problem. Let me outline the issue real quick. I use the meta.description and meta.maintainers to find all the files with descriptions and maintainers via a I then have an I then group all the options according to those files I get. The problem. I am pretty sure graph does not contain an exhaustive list of the modules which may show up in the declared-by field. That AND I also just have a bug in there. But regardless we lose the options from all the modules which do not have a meta.description or meta.maintainers field... so we are losing even some things that actually are in the graph. And the problem is somewhere in normopts.nix in the associate, modules-by-meta or groupByDecl sections most likely, unless we just are not getting enough info from the graph... But I think it might be some of both. Basically, its very much nontrivial, and this was a lot of help already, and the code there is fairly compact and does a lot of processing and deals with a lot of internal module stuff, so I wouldn't fault you for not wanting to mess with it until I either clean it up more or make it work or both. The sidebar is a lot of help already, and will make a big difference. But if some really hard nix mixed attrset/list parsing is what sounds fun to you, and you want to spend it figuring out how to collect those, I have had that mood before, I won't stop you. I was waiting for a time where that urge lines up with some free time to tackle it but I have had plenty to do in the meantime which has kept me from doing it mostly. |
|
Anyway, that aside, let me know when this is ready to merge.
Would you mind explaining this a bit more? Do you mean they meant li.classList.remove('expanded'); // remove instead of add
if (level < foldLevel) {
li.classList.add('expanded'); // so that this is what either sets it back or not?
}Regardless, what you have seems to work at least well enough, you add it for the ones which are open in the document and remove it for the ones which are closed? So, that seems to not depend on their default for it? What would happen if they fixed that? Would we need to update our JS? Are you saying that if they fix that, we could set the fold level, and then only open the ones we need to? But what we have should still work either way, correct? |
|
Oh I just realized I didnt answer the 3 questions
Probably. Maybe "this relies on the fact that module headings are the only level 2 headings in details > summary elements?" Something like that.
Yes actually I think that those are good names for them
If you want? But the file is < 300 lines and doesn't have a reason it will be growing massively any time soon so it should be fine for now. They will probably be moved when the docgen is exposed to users either way. I moved the zshrc files because it was getting out of hand in 1 file. Some of them are a bit short now but combined it was just too much scrolling. Especially with the thing with the redefined wrapperImplementation option for the variants that we no longer needed there after my last changes to the makeWrapper module. The only thing I really have to say even remotely negatively now, is that they don't continue to open and close together. But I also understand that doing that is a whole separate thing on top of this involving action listeners and communication between elements... we should save that for later if we even want to bother with that for something like this with the docs. I'm leaning towards not to be honest. They do still open when you click a sidebar item inside a currently closed section. So, this is completely fine and does not bother me at all. Maybe even preferred by some users (less page movement with the sidebar). So I am not even sure if it is actually even an issue whatsoever. It may even be better that it does not do that. It is also, again, just so much better than it was. So, thank you for that. |
|
I have been wanting to dive into something nix related to try and improve my understanding. I will see how my time availability is and let you know. Thanks for the details. Also, if I do work on it a bit and you end up having time to do it yourself I don't mind as the process and then seeing your solution would still be a big benefit to me. |
|
For the JS bug, they are dynamically creating the sidebar elements when you visit a page. The section I quoted is supposed to only add 'expanded' if the new element is for a heading above the fold level, but they add it to everything. You are correct, if it is fixed, we could choose a level so everything is folded and then we only open what we want. The one caveat to that is I am not sure where that fold level is defined as a config, it may be hard coded in their template. (The fold option for the html renderer doesn't seem to affect it) Also, yes, the solution I have here should work even if it is fixed. It would just be doing some unnecessary checks. |
|
I did notice the same behaviour in terms of not opening, and I came to the same conclusion that maybe it is better for the sidebar to be separate. One thing we could do is setup an event listener so that if you click a module heading (to nav to it as opposed to clicking the dropdown arrow to expand) it will open the details for that module. |
|
Ok, well, in my book, it is good to merge now, minus maaaybe the comment, so, whenever you are ready mark the PR ready and we should be good to merge it!
If you mean, place it on the headings in the page, and make clicking that open the sidebar, that would be cool I guess but not required. The other way around works already. I don't think they need to be synced any more than that, and even that is optional. |
2 participants
This is not final, I wanted to get your thoughts on how it is progressing.
Changes
Notes:
Bug
At line 409 in toc.js, the add the expanded class, ignoring the foldLevel constant check.