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

Render TOC in nested pages #261

Merged
merged 8 commits into from
Feb 11, 2019
Merged

Render TOC in nested pages #261

merged 8 commits into from
Feb 11, 2019

Conversation

rizo
Copy link
Collaborator

@rizo rizo commented Dec 3, 2018

Currently the table of contents is only rendered for top-level modules. This PR implements the rendering for all nested items (modules, module types, classes, functors and functor parameters) and makes some other minor adjustments.

Full list of changes

  • Render TOC (if not empty) inside header in all nested pages.
  • Fix a bug where functor arguments were listed in reverse order (diff).
  • Remove the doc CSS class from class items, to be consistent with other nested items (diff).
  • Render functor parameters as ul (and not as a dl).
  • Extend the "nested" HTML test to cover the proposed changes.

@rizo rizo added the output label Dec 3, 2018
@rizo rizo changed the title Render TOC in all nested items Render TOC in nested pages Dec 3, 2018
@aantron
Copy link
Contributor

aantron commented Dec 3, 2018

I suggest...

  • Addressing Test nested HTML pages #213 first.
  • Splitting up the one commit into the individual changes that were made, so each can be reviewed trivially. Note that links to code diffs don't also link to diffs in the test cases, and the net effect of combining everything is to partially defeat the advantage to review offered by having expected output committed to the repo.

I can take care of #213, but please let me know if you have intent to do it.

@rizo
Copy link
Collaborator Author

rizo commented Dec 3, 2018

Thanks for the suggestions and apologies for bundling everything (the changes seemed rather small). I changed the history to reflect the individual changes accordingly.

Addressing #213 first.

There's a large number of other nested files (40+) that are not currently tested. Although I agree that #213 must be fixed, I don't see why this PR should be blocked until then. I can add the generated nested expect files if that helps for now.

I'm currently working on other PRs, so feel free to work on #213.

@aantron aantron mentioned this pull request Dec 10, 2018
2 tasks
@rizo
Copy link
Collaborator Author

rizo commented Dec 10, 2018

It is possible to test this now! I'll add the generated files later today.

@rizo
Copy link
Collaborator Author

rizo commented Dec 10, 2018

I included the latest changes from #266 so the tests should work now. 409e0d3 adds the untracked HTML files.

@leostera leostera mentioned this pull request Feb 8, 2019
Copy link
Collaborator

@leostera leostera left a comment

Choose a reason for hiding this comment

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

I've re-run CI and it's green ✅ — from what I understand, the changeset looks good to me, and the output seems to be good too:

screenshot 2019-02-08 at 13 09 32

@rizo
Copy link
Collaborator Author

rizo commented Feb 8, 2019

@aantron I think this can be merged now, unless you have any objections/suggestions.

@dbuenzli dbuenzli merged commit 67a856d into ocaml:master Feb 11, 2019
@rizo rizo mentioned this pull request Feb 11, 2019
@rizo
Copy link
Collaborator Author

rizo commented Feb 11, 2019

Thanks, @dbuenzli.

I think @aantron had some potential suggestions with regards to testing in this PR. @aantron, if that's the case, please let me know and I'll work on that separately.

@rizo rizo mentioned this pull request Feb 11, 2019
2 tasks
@rizo rizo deleted the render-nested-toc branch February 11, 2019 12:01
@aantron
Copy link
Contributor

aantron commented Feb 11, 2019

Yes. If the changes are trivial, I'll just make them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants