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 configuration to enable ordered list #117

Merged
merged 1 commit into from
Oct 12, 2020

Conversation

zsyed91
Copy link
Contributor

@zsyed91 zsyed91 commented Jun 26, 2020

Overview

By default jekyll-toc generates the HTML using unordered lists. If users desire ordered lists for their table of contents they cannot do so easily. With the changes below, the default behavior of using unordered lists will remain. However a new configuration option will toggle between <ul></ul> and <ol></ol> tags accordingly.

With the changes, users can add the existing custom classes to the top level heading and sub-headings separately. That way they can style the top heading and sub-headings separately. In this pass, I did not add the functionality to style multiple nested sub-headings but rather just all sub-headings using the existing configuration. If we feel that this would be useful, we can extend the current logic in the PR to inject classes similar to the list items so end users can style each sub-heading separately. This might be overkill so for simplicity, I have left it to just top level heading and sub-headings.

Changes in PR

  • Created new configuration option to enable ordered list through use_ordered_list input
  • Default behaviour is false so users will continue to have ul tags
  • Using existing class configuration, user can change styling on top ol tag and sub listing ol tags separately
  • No ability to update styling on sub-nested ol tags separately but code is open to this change
  • Added tests for new logic
  • Update README with example of usage and render

Related Issues

* Created new configuration option to enable ordered list through use_ordered_list input
* Default behaviour is false so users will continue to have ul tags
* Using existing class configuration, user can change styling on top ol tag and sub listing ol tags separately
* No ability to update styling on sub-nested ol tags separately but code is open to this change
* Added tests for new logic
* Update README with example of usage and render
@zsyed91
Copy link
Contributor Author

zsyed91 commented Jun 26, 2020

I totally missed that another user opened this exact issue #71. Hope that this PR will be considered! Also I am not sure why the builds above are not running?

@zsyed91
Copy link
Contributor Author

zsyed91 commented Jul 13, 2020

@toshimaru friendly ping

@toshimaru
Copy link
Owner

@zsyed91 Sorry, I totally missed your ping.
I'm gonna review this in this week for sure. 👍

@toshimaru
Copy link
Owner

toshimaru commented Aug 31, 2020

I'd say the code base is a bit old? (That's why the CI doesn't run, I guress)
Could you rebase the code?

@toshimaru toshimaru self-requested a review August 31, 2020 10:54
Copy link
Owner

@toshimaru toshimaru left a comment

Choose a reason for hiding this comment

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

@zsyed91 I've reviewed! 👍

lib/table_of_contents/configuration.rb Show resolved Hide resolved
test/parser/test_ordered_list.rb Show resolved Hide resolved
lib/table_of_contents/parser.rb Show resolved Hide resolved
@toshimaru
Copy link
Owner

Seems no response for my review, but I'm gonna take this PR.
Thanks @zsyed91

@toshimaru toshimaru merged commit d6c4d04 into toshimaru:master Oct 12, 2020
@toshimaru toshimaru mentioned this pull request Oct 12, 2020
@zsyed91
Copy link
Contributor Author

zsyed91 commented Oct 12, 2020 via email

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.

None yet

2 participants