Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#15 | 60e145bfb823784d3d814a7529701e55748bb404...use-field-formatter.patch | 17.29 KB | grasmash |
Comments
Comment #2
grasmash CreditAttribution: grasmash at Acquia commentedComment #3
grasmash CreditAttribution: grasmash at Acquia commentedComment #4
grasmash CreditAttribution: grasmash at Acquia commentedI've resolved these issues in a fork of the module: https://github.com/grasmash/drupal-table-of-contents
Changes include:
Comment #5
grasmash CreditAttribution: grasmash at Acquia commentedComment #6
e0ipso@grasmash is there any chance you can provide a patch? Or better yet, would you like to become a co-maintainer of the module?
Comment #7
grasmash CreditAttribution: grasmash at Acquia commentedI'd be happy to co-maintain.
Diff here:
https://github.com/grasmash/drupal-table-of-contents/compare/60e145bfb82...
Patch here:
https://github.com/grasmash/drupal-table-of-contents/compare/60e145bfb82...
Also patch is attached.
Comment #8
grasmash CreditAttribution: grasmash at Acquia commentedAttached wrong patch. Fixing.
Comment #9
grasmash CreditAttribution: grasmash at Acquia commented@e0ipso are you able to grant me maintainership?
Comment #10
e0ipso@grasmash sorry for the delay. And thanks for your patience. I will find a time today to review the code and grant you privileges.
Comment #11
e0ipsoSome preliminary comments:
Why are we losing this? How do you approach this problem?
It seems you are disregarding these comments. AFAIK this was causing problems and I had to change to this implementation.
Comment #12
grasmash CreditAttribution: grasmash at Acquia commentedThis 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?
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.
Comment #13
grasmash CreditAttribution: grasmash at Acquia commentedI 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...
Comment #14
grasmash CreditAttribution: grasmash at Acquia commentedGiven 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.
Comment #15
grasmash CreditAttribution: grasmash at Acquia commentedComment #16
e0ipsoThanks @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!
Comment #17
grasmash CreditAttribution: grasmash at Acquia commentedThank 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.
Comment #18
e0ipsodone!
Comment #20
aitala CreditAttribution: aitala commentedIs 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