search

tscanlin / tocbot

Build a table of contents from headings in an HTML document.
MIT License
1.39k stars 115 forks source link

`tocElement` option only works if `tocSelector` selects the same element #266

Closed BenceSzalai closed 2 years ago

BenceSzalai commented 2 years ago

Hi! I'm planning to submit a PR for an unrelated functionality, but I've just stumbled upon this recent feature to use an HTMLElement (tocElement) instead of a selector (tocSelector). It looks like to me that this feature is incomplete, isn't it? In many places inside build-html.js the code still relies solely on document.querySelector(options.tocSelector) and it does not examine if tocElement is defined instead. E.g.: https://github.com/tscanlin/tocbot/blob/master/src/js/build-html.js#L134 https://github.com/tscanlin/tocbot/blob/master/src/js/build-html.js#L182 https://github.com/tscanlin/tocbot/blob/master/src/js/build-html.js#L198 https://github.com/tscanlin/tocbot/blob/master/src/js/build-html.js#L203 https://github.com/tscanlin/tocbot/blob/master/src/js/build-html.js#L210 https://github.com/tscanlin/tocbot/blob/master/src/js/build-html.js#L222

So I've tested and it looks like the thing only works with tocElement, if the element passed still can be matched using the tocSelector. If no tocSelector is specified, the default of .js-toc is applied. So this only works as long as the passed tocElement matches the document.querySelector(options.tocSelector), should options.tocSelector be defined or default.

See for yourself here: https://codepen.io/sbnc_eu/pen/YzExVeq It gives a warning: Element not found: .js-toc and the TOC is missing Uncomment line 3 in the JS section to get it working.

It is not even caught by the tests, because on one hand the test HTML have the default js-toc class on the target element so it'd work even if tocElement was tested, on the other hand because the tocElement option is not even tested, all tests run with the default options regarding tocSelector / tocElement, i.e. using tocSelector.

As a workaround one can always just set tocSelector to a selector that matches tocElement, the only question is than what the tocElement option is good for?

Also the same applies regarding contentSelector vs. contentElement.