I found the implementation of this module problematic in a few ways.

  • There was an undocumented requirement to use both a text formatter (to output in-text anchors) and a field formatter setting. This seemed like a missed opportunity to just use a field formatter for both TOC generation and anchor generation.
  • There seemed to be useless JS included
  • Anchor ids were non-semantic
  • Only h2 tags had anchors generated, despite configuration.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

grasmash created an issue. See original summary.

grasmash’s picture

Issue summary: View changes
grasmash’s picture

Issue summary: View changes
grasmash’s picture

I've resolved these issues in a fork of the module: https://github.com/grasmash/drupal-table-of-contents

Changes include:

  • This module now provides a new "Text with anchors & table of contents" field formatter.
  • The module no longer provides a text formatter
  • Anchors are automatically added to displayed field text
  • JS has been removed
  • Anchor names are now generated from the text contents of the selector
grasmash’s picture

e0ipso’s picture

@grasmash is there any chance you can provide a patch? Or better yet, would you like to become a co-maintainer of the module?

grasmash’s picture

Status: Active » Needs review
FileSize
19.27 KB
grasmash’s picture

FileSize
13.65 KB

Attached wrong patch. Fixing.

grasmash’s picture

@e0ipso are you able to grant me maintainership?

e0ipso’s picture

@grasmash sorry for the delay. And thanks for your patience. I will find a time today to review the code and grant you privileges.

e0ipso’s picture

Some preliminary comments:

  1. diff --git a/table_of_contents.js b/table_of_contents.js
    deleted file mode 100644
    

    Why are we losing this? How do you approach this problem?

  2. +++ b/table_of_contents.module
    @@ -271,48 +277,14 @@ function _table_of_contents_get_elements($field_data, $entity_type, $entity_id,
    -  // We can't use drupal_html_id here since it keeps track of ids
    -  // on the page, and we're having to use check_markup on certain
    -  // input formats. This is a bit hacky, but it'll have to do for
    -  // now.
    ...
    -  // Make sure there the id is not too long and unique.
    

    It seems you are disregarding these comments. AFAIK this was causing problems and I had to change to this implementation.

grasmash’s picture

Why are we losing this? How do you approach this problem?

This didn't appear to provide any value. It adds ids to fields, but those ids have no corresponding list items in the table of contents, so they're effectively invisible and non-functional in the context of a table of contents. Maybe I missed something?

- // We can't use drupal_html_id here since it keeps track of ids
- // on the page, and we're having to use check_markup on certain

I'm using drupal_clean_css_identifier() rather than drupal_html_id(). It does not keeps track of ids and so does not encounter any issues. If you're concerned about uniqueness, we can track the generated ids and append suffixes. At the moment, this method has the advantage of generating semantic ids.

grasmash’s picture

I just added a commit that will ensure that HTML ids are unique while preserving semantic ids.

https://github.com/grasmash/drupal-table-of-contents/commit/de2e53af89f6...

grasmash’s picture

Given the new implementation, I don't think we will ever encounter a situation in which a header has no ID, so table_of_contents.js should no longer be necessary.

grasmash’s picture

e0ipso’s picture

Status: Needs review » Reviewed & tested by the community

Thanks @grasmash.

I'm going to grant you commit access. Let's start a 7.x-2.x branch for this! Exciting! Feel free to commit, but please keep opening issues with patches rather than push directly without notification.

Thanks for your patience!

grasmash’s picture

Thank you!

I've created a 7.x-2.x branch and pushed it. Can you please add the 7.x-2.x-dev release, or grant me access to administer releases?

We should also add new instructions for the 7.x-2.x to the project page.

e0ipso’s picture

Status: Reviewed & tested by the community » Fixed

done!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

aitala’s picture

Is this branch usable? I've run into the issue with watchdog filling up with "A node without an ID was found in entity..." messages.
For the moment, I've just commented out the call to write to the log...

Thanks,
Eric