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 automatically-generated sidebar for documentation #3652

Merged
merged 23 commits into from
Mar 22, 2024

Conversation

rcurtin
Copy link
Member

@rcurtin rcurtin commented Mar 7, 2024

I found that the automatically generated documentation was a little bit hard to browse, especially because some pages got really long. I thought, it would be nice to add a sidebar for easier navigation throughout the whole documentation! Here it is in action, when looking at core.html:

image

You can browse a fully-built version of this documentation at https://www.ratml.org/misc/mlpack-markdown-doc/ .

The subsections are collapsible using <details> blocks. Unfortunately, that means that the constant part of the sidebar (the "Docs Index" section) needs to be maintained in HTML and not Markdown; this file is added as doc/sidebar.html.

Then, the second part is custom-generated based on the h1/h2/h3 blocks in each individual page, according to these rules:

  • If the file is user/methods/*.md, then h2s are used for the title and h3s are used for the sections on the sidebar
  • If the file is user/core.md, then h1 is used for the title, h2s are used for the outer list, and h3s are used for the collapsible details sublists
  • Otherwise, h1 is used for the title and h2s are used for the outer list

For longer-term maintainability, my imagination here is that the scripts that generate the sidebar shouldn't need to be updated often (if at all), and whenever a new method is added or index.md is modified, the sidebar will need to be modified too. Therefore, I've made a note of that at the top of index.md.

This PR can't really be reviewed correctly until #3639 is merged, so it's not worth looking at the diff until then.

@shrit
Copy link
Member

shrit commented Mar 13, 2024

@rcurtin is this ready for review?

@rcurtin
Copy link
Member Author

rcurtin commented Mar 13, 2024

@shrit yeah, please go ahead 👍

Copy link
Member

@shrit shrit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some of these algorithms are not part of the link that you have shared. I do not know why they are not appearing in the sidebar, is this intended?

Also, it took me some time to understand that the triangle on the right side of the classification word is clickable. To be honest I was not able to click on it the first time.

Screenshot 2024-03-13 at 16-29-30 mlpack documentation

In addition, from a user experience perspective, the words on the sidebar are really small, I know we are still young, but if you have a screen with a high resolution, it is annoying and requires a lot of focus to read with it written.
In my opinion, the sidebar should occupy the entire length of the screen and when you go down it should also follow.

Screenshot 2024-03-13 at 16-30-51 mlpack documentation

Final note, when clicking on any method inside the classification section, the sidebar reset to its default status. It would be nice for it to show where are we right now, to facilitate the navigation between pages.

These are all my comments, otherwise, this is starting to look really good right now and I really like the first step page, with all the complicated steps and commands, it make the entire library feels easy to work with.

Comment on lines +102 to 105
| `datasetInfo` | [`data::DatasetInfo`](../load_save.md#loading-categorical-data) | Dataset information, specifying type information for each dimension. | _(N/A)_ |
| `labels` | [`arma::Row<size_t>`](../matrices.md) | Training labels, [between `0` and `numClasses - 1`](../load_save.md#normalizing-labels) (inclusive). Should have length `data.n_cols`. | _(N/A)_ |
| `dimensionality` | `size_t` | When using on numeric-only data, this specifies the number of dimensions in the data. | _(N/A)_ |
| `numClasses` | `size_t` | Number of classes in the dataset. | _(N/A)_ |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not see the hoeffding tree anywhere in the list, not on the bar or in the main page. Any reason for this disappearance?

Comment on lines 45 to 50

* [mlpack regression techniques](#mlpack_regression_techniques) <!-- TODO: fix link! -->
* [`LinearRegression`](linear_regression.md) <!-- TODO: fix link -->
* [`LARS`](lars.md) <!-- TODO: fix link -->
* [mlpack regression techniques](../../index.md#regression-algorithms)
* [`LinearRegression`](linear_regression.md)
* [`LARS`](lars.md)
* [Bayesian linear regression on Wikipedia](https://en.wikipedia.org/wiki/Bayesian_linear_regression)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bayesian linear regression is not anywhere on the list

@@ -28,7 +26,7 @@ information...](#fully-custom-behavior))
arma::mat dataset(10, 1000, arma::fill::randu); // 1000 points.
arma::Row<size_t> labels =
arma::randi<arma::Row<size_t>>(1000, arma::distr_param(0, 4));
arma::mat testDataset(10, 500, arma::fill::randu); // 500 test points.
arma::mat testData(10, 500, arma::fill::randu); // 500 test points.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RF is not anywhere as well

@rcurtin
Copy link
Member Author

rcurtin commented Mar 13, 2024

Some of these algorithms are not part of the link that you have shared. I do not know why they are not appearing in the sidebar, is this intended?

Nope, not intended! I just overlooked them. I added HoeffdingTree, LinearSVM, and RandomForest to sidebar.html in bc79342. Thanks for pointing that out, I had totally missed them 👍

If you refresh now, those are in the sidebar.

Also, it took me some time to understand that the triangle on the right side of the classification word is clickable. To be honest I was not able to click on it the first time.

This one is a little tricky. I used the HTML5 <summary> component for this, because I didn't want any fancy JS or anything like that. My thought was that even if a user doesn't realize that they can expand the section, they can just click on the title and it takes them somewhere that has all the subsections anyway. So I thought maybe it doesn't matter...

One easy fix I have is to change the character to something else with simple CSS. Do you think maybe a [+] and [-] are better? I could try that. Past that it gets tricky, some more thoughts below:

In my opinion, the sidebar should occupy the entire length of the screen and when you go down it should also follow.

If we expand all the sections by default (or don't allow them to collapse), you can end up with a sidebar that's longer than the length of the page. How does the user navigate the sidebar then? They would need a scrollbar specifically for the sidebar... I'd really like to avoid that, then it feels like the page has frames. My thinking was that with collapsible sections, a user would just collapse things if the sidebar was too long for their browser.

For an example of a page where the sidebar will get very long, try expanding all the sections on this one:

https://www.ratml.org/misc/mlpack-markdown-doc/user/core.html

More and more will get added to there as time goes on, and that sidebar will get really long.

Final note, when clicking on any method inside the classification section, the sidebar reset to its default status. It would be nice for it to show where are we right now, to facilitate the navigation between pages.

I thought so too but I don't know an easy way to do this. When you click any link on the sidebar, it sends you to a totally new page, and so I would need to somehow encode the sidebar state to be passed to the new page. I think it is possible but I don't know how to do it easily. Maybe we can save the idea for later, or maybe someone else has an idea?

In addition, from a user experience perspective, the words on the sidebar are really small, I know we are still young, but if you have a screen with a high resolution, it is annoying and requires a lot of focus to read with it written.

I used 85% for the font size there. Is that super small on your screen? On my screen it is only a tiny bit smaller, and on small screens like phones the sidebar won't appear (try making your browser narrower, you will see that it goes away).

My main concern was that if the font size is the same as the main page, it gets a bit harder visually to differentiate between the sidebar and the main content. 90% could work. If you want we could hop on a call and screw with it by changing around CSS in your browser, maybe you are seeing something totally different than I am?

@shrit
Copy link
Member

shrit commented Mar 13, 2024

I refreshed the page. However I do not see them on the main page, the new methods are in the classification but not in the main page.

It is really small, I have added a full-screen capture to give you insight

Screenshot 2024-03-13 at 23-15-23 mlpack documentation

I would not mind it having the same size, there is a lot of space between the sidebar and the main page in the middle.

is there any way to make the font adjust automatically in the sidebar so the size of the sidebar would not need to be navigated using a scrollbar and would be always on the same page even if everything is open?

Another solution would be to have a dynamic list. Once you open one the other closes automatically, but I think you need JS for this

@shrit
Copy link
Member

shrit commented Mar 13, 2024

[+] and [-] could be better, maybe you can give it a try locally and see if it is easier to use ?

@rcurtin
Copy link
Member Author

rcurtin commented Mar 19, 2024

@shrit can you tell me what you think of the changes I made? http://ratml.org/misc/mlpack-markdown-doc/ I upped the font size in the sidebar a little bit, allowed it to be wider when possible, and changed to [+] and [-] for the collapsible sections. I think all those things are improvements for sure.

@shrit
Copy link
Member

shrit commented Mar 19, 2024

@rcurtin looks better, Is there any reason why all the methods are not appearing here:
Screenshot 2024-03-19 at 21-43-09 mlpack documentation

@rcurtin
Copy link
Member Author

rcurtin commented Mar 21, 2024

Ahh, nice catch, I didn't realize the index.md file was missing some algorithms too. Fixed now in ac114f2.

Copy link

@mlpack-bot mlpack-bot bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Second approval provided automatically after 24 hours. 👍

@rcurtin rcurtin merged commit 24e6c93 into mlpack:master Mar 22, 2024
18 of 20 checks passed
@rcurtin rcurtin deleted the doc-sidebar branch March 22, 2024 15:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants