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

Use tocify JS for dynamic TOC [upstream only] #3453

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Lennonka
Copy link
Contributor

@Lennonka Lennonka commented Nov 13, 2024

What changes are you introducing?

Making the table of contents expandable and collapsible.

Why are you introducing these changes? (Explanation, links to references, issues, etc.)

Improves UX for Foreman Docs.
For long documents, it's more user friendly and we will be able to display level-3 headings.

Anything else to add? (Considerations, potential downsides, alternative solutions you have explored, etc.)

Checklists

  • I am okay with my commits getting squashed when you merge this PR.
  • I am familiar with the contributing guidelines.

Please cherry-pick my commits into:

  • Foreman 3.13/Katello 4.15 (Satellite 6.17)
  • Foreman 3.12/Katello 4.14 (Satellite 6.16)
  • Foreman 3.11/Katello 4.13 (orcharhino 6.11 on EL8 only)
  • Foreman 3.10/Katello 4.12
  • Foreman 3.9/Katello 4.11 (Satellite 6.15; orcharhino 6.8/6.9/6.10)
  • Foreman 3.8/Katello 4.10
  • Foreman 3.7/Katello 4.9 (Satellite 6.14)
  • Foreman 3.6/Katello 4.8
  • Foreman 3.5/Katello 4.7 (Satellite 6.13; orcharhino 6.6/6.7)
  • We do not accept PRs for Foreman older than 3.5.

@Lennonka Lennonka changed the title Use tocify JS for dynamic TOC Use tocify JS for dynamic TOC [upstream only] Nov 13, 2024
Copy link

github-actions bot commented Nov 13, 2024

The PR preview for 667a96b is available at theforeman-foreman-documentation-preview-pr-3453.surge.sh

The following output files are affected by this PR:

show diff

show diff as HTML

@Lennonka Lennonka marked this pull request as draft November 13, 2024 21:33
@Lennonka Lennonka force-pushed the expandable-toc-for-foreman-docs branch from 6f0bed8 to 95cac8f Compare November 13, 2024 21:49
@Lennonka Lennonka marked this pull request as ready for review November 13, 2024 21:51
@Lennonka
Copy link
Contributor Author

A few tweaks and increased levels of headings.

It's actually quite cool! 😎

Copy link
Contributor

@maximiliankolb maximiliankolb left a comment

Choose a reason for hiding this comment

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

one suggestion.

Comment on lines 2 to 4
<script src="https://code.jquery.com/jquery-1.11.3.min.js"></script>
<script src="https://code.jquery.com/ui/1.11.4/jquery-ui.min.js"></script>
<script src="https://cdnjs.cloudflare.com/ajax/libs/jquery.tocify/1.9.0/javascripts/jquery.tocify.min.js"></script>
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be cool if we checkin these scripts. They are ~350kb in total which is a lot, but I'd be ok with that.

@Lennonka Lennonka force-pushed the expandable-toc-for-foreman-docs branch from 95cac8f to 667a96b Compare November 14, 2024 18:27
@Lennonka
Copy link
Contributor Author

Sourcing the JS libraries locally. And rebased.

They have the MIT license - is that compatible with us?

I hope I managed to get the JS path correctly.

Comment on lines +2 to +4
<script src="/js/jquery-1.11.3.min.js"></script>
<script src="/js/jquery-ui.min.js"></script>
<script src="/js/jquery.tocify.min.js"></script>
Copy link
Contributor Author

@Lennonka Lennonka Nov 14, 2024

Choose a reason for hiding this comment

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

This JS path should be correct because the same path is used to call /js/nav.js but the preview isn't working now and I'm not sure why.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ekohl Would you be able to help?

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

Successfully merging this pull request may close these issues.

2 participants