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

Added options "headings" and "class" #26

Closed
wants to merge 1 commit into from

Conversation

Ircama
Copy link
Contributor

@Ircama Ircama commented Oct 17, 2016

Added options "headings" and "class"

Congratulations for this plugin and thanks!

This PR would like to contribute by adding the following options:

  • headings
  • class

Example here (unfinished document at the time of writing).

I also updated index.md (the documentation) and I set the version to "0.4.2".

Feel free to revise the code as you like.

Notice that with this PR tests and CDN are not updated.

Changes to be committed:
modified: bootstrap-toc.js
modified: dist/bootstrap-toc.js
modified: dist/bootstrap-toc.min.js
modified: index.md
modified: package.json

Congratulations for this plugin and thanks!

This PR would like to contribute by adding the following options:

 * headings
 * class

Example [here](https://ircama.github.io/osm-carto-tutorials/tile-server-ubuntu/) (unfinished document at the time of writing).

I also updated index.md (the documentation) and I set the version to "0.4.2".

Feel free to revise the code as you like.

Notice that with this PR tests and CDN are not updated.

 Changes to be committed:
	modified:   bootstrap-toc.js
	modified:   dist/bootstrap-toc.js
	modified:   dist/bootstrap-toc.min.js
	modified:   index.md
	modified:   package.json
Copy link
Owner

@afeld afeld left a comment

Choose a reason for hiding this comment

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

Hey there! Thanks for submitting. Left some comments inline. While I would be happy to merge some parts of the changes (the description of the search logic and the automatic addition of class names), I'm less sure about the new options. Also, just added some contributor guidelines—even though it's after the fact, mind giving them a read and updating accordingly? Thanks again!

headings: 'h1,h2,h3,h4,h5,h6',
class: 'my-class'
});
```
Copy link
Owner

Choose a reason for hiding this comment

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

Let's use the existing options object—no need to make a second one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK. Would you please suggest a code rework? Thanks

Copy link
Owner

Choose a reason for hiding this comment

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

Toc.init({
  $nav: $myNav,
  headings: 'h1,h2,h3,h4,h5,h6',
  class: 'my-class'
});

@@ -154,6 +154,72 @@ nav[data-toggle='toc'] {
}
```

## Additional options

By default, the plugin finds the first heading level (`<h1>`, then `<h2>`, etc.) that has more than one element and defaults to 1 (for `<h1>`). The identified level becomes the top heading and the plugin supports a single nesting level associated to it, including the subsequent heading only. E.g., if the identified top level is `<h1>`, the nested level will be `<h2>`, which is the next below it; if the top level is `<h2>`, the nested one will be `<h3>`. No additional level to the nested one and no previous level to the top one will be shown.
Copy link
Owner

Choose a reason for hiding this comment

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

This is great, thanks for adding!

Copy link
Contributor Author

@Ircama Ircama Oct 24, 2016

Choose a reason for hiding this comment

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

I added it as it took me sometimes to understand that, if my top level was h2, only this level and h3 could be represented, while h1 within the middle of the document and also h4 did not show up.


The plugin allows options to customize this behavior:

* `headings` (which can be a string): if set to a list of headers (in the form *hN,hM,...* where N, M, ... are levels, e.g., `h1,h2,h3`), all of them are searched instead of the first declared one and of its subsequent heading. Example: `headings: 'h1,h2,h3,h4,h5,h6'` (in this case, all of them are searched and not only `<h2>` and `<h3>` in case `<h2>` is the top level).
Copy link
Owner

Choose a reason for hiding this comment

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

What's the use case here? Do you have an example page where you would want to use this? What would the desired outcome be? Also, does this differ from

$scope: $('h1,h2,h3,h4,h5,h6')

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi, I generally agree about your comments and you can add new commits to this branch. This PR was more than discuss than considering an immediate merge.

What's the use case here? Do you have an example page where you would want to use this? What would the desired outcome be?

Check this page as an example:

https://ircama.github.io/osm-carto-tutorials/tile-server-ubuntu/

Your plugin is running there.

In fact, at the end of the HTML:

  $(function() {
    var navSelector = '#toc';
    var $myNav = $(navSelector);
    Toc.init($myNav, { headings: 'h1,h2,h3,h4,h5,h6' } );
    $('body').scrollspy({
      target: navSelector,
      offset: 220
    });
  });

In the page, you can see that my top level is 2 and that I generally use one sublevel, but there are some paragraphs with an additional sublevel 4:

https://ircama.github.io/osm-carto-tutorials/tile-server-ubuntu/#install-mapnik-library-from-package

  • [h2] Install Mapnik library
    • [h3] Install Mapnik library from package
    • [h3] Alternatively, install Mapnik from sources
      • [h4] Install Boost from package
      • [h4] Alternatively, install the latest version of Boost from source
      • [h4] Install HarfBuzz from source
      • [h4] Build the Mapnik library from source

You see that h4 items are present (this is thanks to { headings: 'h1,h2,h3,h4,h5,h6' }), are jointly expanded with h3 (and there is no way with this drop to do differently) and they are indented and represented with smaller font size (thanks to the auto-class naming and to the CSS).

Also, does this differ from $scope: $('h1,h2,h3,h4,h5,h6')?

Could you please make an example of this?

Copy link
Owner

Choose a reason for hiding this comment

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

Let's deal with the "depth" feature request separately: #37

The plugin allows options to customize this behavior:

* `headings` (which can be a string): if set to a list of headers (in the form *hN,hM,...* where N, M, ... are levels, e.g., `h1,h2,h3`), all of them are searched instead of the first declared one and of its subsequent heading. Example: `headings: 'h1,h2,h3,h4,h5,h6'` (in this case, all of them are searched and not only `<h2>` and `<h3>` in case `<h2>` is the top level).
* `class`: (which can be a string): if set to a string, its value is used to customize the CSS class that the plugin sets for each description, concatenating it with the related header level. If not set, the default class heading will be `toc-nav-h`. Example: by default, a top level description (`<h1>`) has class`toc-nav-h1`, a second level description (`<h2>`) has class `toc-nav-h2`, etc. If the option `class` is set to e.g. `my-class`, a top level description (`<h1>`) will have class `my-class-h1`, a second level description (`<h2>`) will have class `my-class-h2`, etc. This feature allows adding CSS attributes to customize the descriptions according its header level.
Copy link
Owner

Choose a reason for hiding this comment

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

While I like the adding of classes to different levels to make styling easier, the customization of said class seems overkill...

Copy link
Owner

Choose a reason for hiding this comment

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

Also, what's the behavior when the top-level heading in the sidebar corresponds to an <h2>? My assumption is that users would care more about the level in the table of contents than what h* it corresponds to, so I would make it more clear by using toc-nav-level-1 and toc-nav-level-2 or something like that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While I like the adding of classes to different levels to make styling easier, the customization of said class seems overkill...

Maybe it is indead an overkill if no one is going to use the same root in other places. You can suggest a code rework or do it yourself if wanted.

Also, what's the behavior when the top-level heading in the sidebar corresponds to an <h2>? My assumption is that users would care more about the level in the table of contents than what h* it corresponds to, so I would make it more clear by using toc-nav-level-1 and toc-nav-level-2 or something like that.

If you have top level h2, h1 in the middle of the document will be always shown when setting headings: 'h1,h2,h3,h4,h5,h6', but not shown if leaving this option out.

so I would make it more clear by using toc-nav-level-1 and toc-nav-level-2 or something like that.

To make sure I got your point, would you change toc-nav-h to toc-nav-level-h?

Copy link
Owner

Choose a reason for hiding this comment

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

Still not sure I understand the use case here. Feel free to open an issue if you are still interested.

});
}
catch(e) {
}
Copy link
Owner

Choose a reason for hiding this comment

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

Mind removing the try/catch? Using it like this would swallow any errors, and therefore is definitely not something I want the documentation to encourage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, right!

*
* Modified by Ircama, 2016, adding the following options:
* - headings
* - class
Copy link
Owner

Choose a reason for hiding this comment

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

Mind removing this? If all contributions were added to the top of the file, it would get very large very fast 😏 Easier to point people to

https://github.com/afeld/bootstrap-toc/search?q=-author%3Aafeld&type=Issues&utf8=%E2%9C%93

since that list keeps itself up-to-date.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, sure!

catch(e) {
}
</script>
```
Copy link
Owner

Choose a reason for hiding this comment

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

Thinking it would make more sense to throw an example like this in a sandbox so that it can be interactive, and tweakable by the user.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that would be great, I did not have time to setup it.

afeld pushed a commit that referenced this pull request Jan 9, 2019
Copy link
Owner

@afeld afeld left a comment

Choose a reason for hiding this comment

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

So sorry it took me so long to get back to this. Pulled the documentation additions out in #51 - going to close the rest as stale, but feel free to respond here or open new (smaller) issues / pull requests if still interested. Thanks!


The plugin allows options to customize this behavior:

* `headings` (which can be a string): if set to a list of headers (in the form *hN,hM,...* where N, M, ... are levels, e.g., `h1,h2,h3`), all of them are searched instead of the first declared one and of its subsequent heading. Example: `headings: 'h1,h2,h3,h4,h5,h6'` (in this case, all of them are searched and not only `<h2>` and `<h3>` in case `<h2>` is the top level).
Copy link
Owner

Choose a reason for hiding this comment

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

Let's deal with the "depth" feature request separately: #37

headings: 'h1,h2,h3,h4,h5,h6',
class: 'my-class'
});
```
Copy link
Owner

Choose a reason for hiding this comment

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

Toc.init({
  $nav: $myNav,
  headings: 'h1,h2,h3,h4,h5,h6',
  class: 'my-class'
});

The plugin allows options to customize this behavior:

* `headings` (which can be a string): if set to a list of headers (in the form *hN,hM,...* where N, M, ... are levels, e.g., `h1,h2,h3`), all of them are searched instead of the first declared one and of its subsequent heading. Example: `headings: 'h1,h2,h3,h4,h5,h6'` (in this case, all of them are searched and not only `<h2>` and `<h3>` in case `<h2>` is the top level).
* `class`: (which can be a string): if set to a string, its value is used to customize the CSS class that the plugin sets for each description, concatenating it with the related header level. If not set, the default class heading will be `toc-nav-h`. Example: by default, a top level description (`<h1>`) has class`toc-nav-h1`, a second level description (`<h2>`) has class `toc-nav-h2`, etc. If the option `class` is set to e.g. `my-class`, a top level description (`<h1>`) will have class `my-class-h1`, a second level description (`<h2>`) will have class `my-class-h2`, etc. This feature allows adding CSS attributes to customize the descriptions according its header level.
Copy link
Owner

Choose a reason for hiding this comment

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

Still not sure I understand the use case here. Feel free to open an issue if you are still interested.

@afeld afeld closed this Jan 9, 2019
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