-
Notifications
You must be signed in to change notification settings - Fork 8
Add H1-H6 heading level selection controls to Dynamic Table of Contents block #101
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
base: trunk
Are you sure you want to change the base?
Conversation
WalkthroughThis change releases version 0.2.0. It updates CHANGELOG.md and readme.txt, and bumps the plugin header version. The block API adds six boolean attributes: includeH1–includeH6 (default true). The editor (edit.js) introduces InspectorControls with ToggleControls for these attributes and binds title editing via RichText. The server render (render.php) outputs a data-heading-levels attribute based on selected headings. The front-end script (view.js) generates and highlights TOC items using the configured heading levels. Minor adjustments include camelCase SVG attributes in index.js and formatting-only changes in package.json. Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🧹 Nitpick comments (9)
dynamic-table-of-contents/CHANGELOG.md (1)
1-1: Changelog entry is minimal; consider adding date and scope.Recommend including a release date and a brief note on BC (defaults remain H1–H6) for clarity.
dynamic-table-of-contents/readme.txt (1)
48-49: Changelog wording is fine; consider adding brief details.Optional: add one more bullet (e.g., “Inspector controls to toggle H1–H6; frontend respects selection; BC default remains all levels.”).
dynamic-table-of-contents/package.json (1)
3-3: Version mismatch with plugin/readme (0.1.9 vs 0.2.0).If you tag this as 0.2.0, align package.json to avoid confusion in zips, CI artifacts, and changelog automation.
Apply this diff if the npm package version should match the plugin version:
- "version": "0.1.9", + "version": "0.2.0",dynamic-table-of-contents/src/edit.js (1)
33-101: Inspector toggles are clear; minor DRY opportunity.Current duplication for six toggles is fine; consider mapping over [1..6] to reduce repetition.
Example approach (illustrative):
+ const levels = [ 1, 2, 3, 4, 5, 6 ]; <PanelBody title={ __( 'Heading Levels', 'dynamic-table-of-contents' ) } initialOpen> - <ToggleControl ...H1... /> - ... - <ToggleControl ...H6... /> + { levels.map( ( n ) => { + const key = `includeH${ n }`; + return ( + <ToggleControl + key={ key } + label={ __( `Include H${ n } headings`, 'dynamic-table-of-contents' ) } + checked={ attributes[ key ] } + onChange={ ( v ) => setAttributes( { [ key ]: v } ) } + /> + ); + } ) } </PanelBody>dynamic-table-of-contents/src/view.js (3)
66-69: Set “active” on the first appended item, not first NodeList index.Index 0 may be a heading without an ID, resulting in no active item.
Apply:
-if ( 0 === index ) { +if ( $headingList.children.length === 0 ) { $latestLink.classList.add( 'active' ); }
20-24: Selector base check.Hard‑coding
.wp-block-post-contentmay miss headings on some themes. If feasible, make the base configurable or broaden toarticle, .entry-content, .wp-block-post-contentwith a safe list.
4-11: Support multiple TOC blocks — scope per-instance instead of single querySelector.Iterate document.querySelectorAll('.wp-block-wpcomsp-dynamic-table-of-contents') and run the logic per-instance: use $tocBlock.querySelector('ul') and read data-heading-levels from that $tocBlock; scope heading selection to $tocBlock.closest('.wp-block-post-content').querySelectorAll(...) (instead of document.querySelectorAll); and replace global link queries with $tocBlock.querySelectorAll('a') / $tocBlock.querySelector(
a[href="#${id}"]) so the IntersectionObserver only toggles links inside the current $tocBlock.Locations: dynamic-table-of-contents/src/view.js — top selector (lines ~4–11) and observer/link queries (~29–35).
I can provide a scoped refactor patch that wraps the current logic in a loop over document.querySelectorAll(...).
dynamic-table-of-contents/src/block.json (2)
32-35: Default title string isn’t localizedDefault attribute values aren’t run through i18n. If the UI/render relies on this default, non‑English sites will see “Table of Contents” in English. Prefer leaving the attribute empty and providing a translated fallback in edit/render via __() or ensuring the editor initializes the value with a translated string.
36-59: Attribute shape works; consider a single array for extensibilitySix booleans are clear and backward‑compatible. Optional: replace (or add, then migrate later) a single attribute like
headingLevels: number[]with default[1,2,3,4,5,6]to reduce API surface and simplify serialization/UI mapping.Example schema change (future refactor):
+ "headingLevels": { + "type": "array", + "items": { "type": "number" }, + "default": [1,2,3,4,5,6] + }, - "includeH1": { "type": "boolean", "default": true }, - "includeH2": { "type": "boolean", "default": true }, - "includeH3": { "type": "boolean", "default": true }, - "includeH4": { "type": "boolean", "default": true }, - "includeH5": { "type": "boolean", "default": true }, - "includeH6": { "type": "boolean", "default": true }Please also verify:
- Version alignment: block.json shows 0.1.9 while the PR claims 0.2.0.
- The front‑end script that consumes
data-heading-levelsis enqueued (either viaviewScripthere or in PHP).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
dynamic-table-of-contents/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
dynamic-table-of-contents/CHANGELOG.md(1 hunks)dynamic-table-of-contents/dynamic-table-of-contents.php(1 hunks)dynamic-table-of-contents/package.json(1 hunks)dynamic-table-of-contents/readme.txt(2 hunks)dynamic-table-of-contents/src/block.json(1 hunks)dynamic-table-of-contents/src/edit.js(1 hunks)dynamic-table-of-contents/src/index.js(1 hunks)dynamic-table-of-contents/src/render.php(1 hunks)dynamic-table-of-contents/src/view.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
dynamic-table-of-contents/src/render.php (1)
dynamic-table-of-contents/src/edit.js (1)
attributes(21-29)
🔇 Additional comments (5)
dynamic-table-of-contents/readme.txt (1)
5-5: Stable tag bump looks good.Matches the plugin header version update to 0.2.0.
dynamic-table-of-contents/dynamic-table-of-contents.php (1)
7-7: Version bump aligns with readme.Header version 0.2.0 matches the readme Stable tag. No further action.
dynamic-table-of-contents/src/edit.js (1)
104-116: Title binding via RichText looks good.Uses correct text domain and placeholder; matches server render title output.
dynamic-table-of-contents/src/index.js (1)
33-35: Correct React SVG attribute casing.fillRule/clipRule is the right JSX form. 👍
dynamic-table-of-contents/src/block.json (1)
30-30: usesContext formatting change is fineNo semantic change; single-line array reads cleanly.
| foreach ( $heading_attributes as $heading_attr ) { | ||
| if ( isset( $attributes[ $heading_attr ] ) && true === $attributes[ $heading_attr ] ) { | ||
| $heading_level = strtolower( str_replace( 'include', '', $heading_attr ) ); | ||
| $heading_levels[] = $heading_level; | ||
| } | ||
| } | ||
|
|
||
| $data_attributes = array( | ||
| 'data-heading-levels' => esc_attr( implode( ',', $heading_levels ) ), | ||
| ); | ||
|
|
||
| $wrapper_attributes = get_block_wrapper_attributes( $data_attributes ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All-levels-off edge case currently falls back to “all levels”.
When all toggles are off, $heading_levels becomes empty, resulting in data-heading-levels="". In view.js, an empty string is falsy and defaults to all levels—contrary to the PR objective (“edge case: all levels toggled off (empty list)”). We need to distinguish:
- Legacy blocks (attributes absent) → default to all levels.
- User explicitly set all false → no headings.
Set the data attribute only when any include* attribute exists; leave it absent for legacy. Keep it present (possibly empty) for new blocks. Pair with the view.js change below.
Apply:
// Prepare data attributes for heading levels
$heading_levels = array();
$heading_attributes = array( 'includeH1', 'includeH2', 'includeH3', 'includeH4', 'includeH5', 'includeH6' );
+$has_any_include_attr = false;
-foreach ( $heading_attributes as $heading_attr ) {
- if ( isset( $attributes[ $heading_attr ] ) && true === $attributes[ $heading_attr ] ) {
- $heading_level = strtolower( str_replace( 'include', '', $heading_attr ) );
- $heading_levels[] = $heading_level;
- }
-}
+foreach ( $heading_attributes as $heading_attr ) {
+ if ( array_key_exists( $heading_attr, $attributes ) ) {
+ $has_any_include_attr = true;
+ if ( true === $attributes[ $heading_attr ] ) {
+ $heading_level = strtolower( str_replace( 'include', '', $heading_attr ) );
+ $heading_levels[] = $heading_level;
+ }
+ }
+}
-$data_attributes = array(
- 'data-heading-levels' => esc_attr( implode( ',', $heading_levels ) ),
-);
-
-$wrapper_attributes = get_block_wrapper_attributes( $data_attributes );
+$wrapper_attributes = $has_any_include_attr
+ ? get_block_wrapper_attributes(
+ array( 'data-heading-levels' => esc_attr( implode( ',', $heading_levels ) ) )
+ )
+ : get_block_wrapper_attributes();🤖 Prompt for AI Agents
In dynamic-table-of-contents/src/render.php around lines 30 to 41, the code
currently always sets data-heading-levels based on $heading_levels which becomes
an empty string when all toggles are explicitly false, causing view.js to treat
it as "all levels"; instead, detect whether any include* attribute keys exist in
$attributes (i.e. distinguish legacy blocks where attributes are absent from new
blocks where includes are present even if false). Only build and pass the
'data-heading-levels' entry in $data_attributes when at least one include*
attribute key exists on the block (if present, set its value to the escaped
implode of $heading_levels, which may be empty for all-false); leave the data
attribute entirely absent for legacy blocks.
| // Get the allowed heading levels from data attribute | ||
| const headingLevelsData = $tocBlock.getAttribute( 'data-heading-levels' ); | ||
| const allowedHeadings = headingLevelsData | ||
| ? headingLevelsData.split( ',' ) | ||
| : [ 'h1', 'h2', 'h3', 'h4', 'h5', 'h6' ]; | ||
|
|
||
| if (entry.isIntersecting) { | ||
| $links.forEach((link) => { | ||
| link.classList.remove('active'); | ||
| }); | ||
| // Build selector for allowed headings | ||
| const headingSelectors = allowedHeadings | ||
| .map( ( level ) => `.wp-block-post-content ${ level }` ) | ||
| .join( ', ' ); | ||
| const $headings = document.querySelectorAll( headingSelectors ); | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix fallback vs. explicit-empty selection.
Empty string should mean “no headings,” not “default to all.” Use null-check (attribute absent) for legacy default; treat empty string as []. Also guard against empty selector.
Apply:
-// Get the allowed heading levels from data attribute
-const headingLevelsData = $tocBlock.getAttribute( 'data-heading-levels' );
-const allowedHeadings = headingLevelsData
- ? headingLevelsData.split( ',' )
- : [ 'h1', 'h2', 'h3', 'h4', 'h5', 'h6' ];
+// Get the allowed heading levels from data attribute.
+// Attribute missing (null) => legacy => default all.
+// Present but empty string => explicit empty => none.
+const headingLevelsData = $tocBlock.getAttribute( 'data-heading-levels' );
+const allowedHeadings =
+ headingLevelsData === null
+ ? [ 'h1', 'h2', 'h3', 'h4', 'h5', 'h6' ]
+ : headingLevelsData.split( ',' ).filter( Boolean );
+
+// No allowed headings selected: render empty and exit.
+if ( allowedHeadings.length === 0 ) {
+ return;
+}
// Build selector for allowed headings
const headingSelectors = allowedHeadings
.map( ( level ) => `.wp-block-post-content ${ level }` )
.join( ', ' );
const $headings = document.querySelectorAll( headingSelectors );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Get the allowed heading levels from data attribute | |
| const headingLevelsData = $tocBlock.getAttribute( 'data-heading-levels' ); | |
| const allowedHeadings = headingLevelsData | |
| ? headingLevelsData.split( ',' ) | |
| : [ 'h1', 'h2', 'h3', 'h4', 'h5', 'h6' ]; | |
| if (entry.isIntersecting) { | |
| $links.forEach((link) => { | |
| link.classList.remove('active'); | |
| }); | |
| // Build selector for allowed headings | |
| const headingSelectors = allowedHeadings | |
| .map( ( level ) => `.wp-block-post-content ${ level }` ) | |
| .join( ', ' ); | |
| const $headings = document.querySelectorAll( headingSelectors ); | |
| // Get the allowed heading levels from data attribute. | |
| // Attribute missing (null) => legacy => default all. | |
| // Present but empty string => explicit empty => none. | |
| const headingLevelsData = $tocBlock.getAttribute( 'data-heading-levels' ); | |
| const allowedHeadings = | |
| headingLevelsData === null | |
| ? [ 'h1', 'h2', 'h3', 'h4', 'h5', 'h6' ] | |
| : headingLevelsData.split( ',' ).filter( Boolean ); | |
| // No allowed headings selected: render empty and exit. | |
| if ( allowedHeadings.length === 0 ) { | |
| return; | |
| } | |
| // Build selector for allowed headings | |
| const headingSelectors = allowedHeadings | |
| .map( ( level ) => `.wp-block-post-content ${ level }` ) | |
| .join( ', ' ); | |
| const $headings = document.querySelectorAll( headingSelectors ); |
| const $observer = new IntersectionObserver( | ||
| ( entries ) => { | ||
| const $links = document.querySelectorAll( | ||
| '.wp-block-wpcomsp-dynamic-table-of-contents a' | ||
| ); | ||
|
|
||
| entries.forEach( ( entry ) => { | ||
| const $id = entry.target.id; | ||
| const $link = document.querySelector( | ||
| `.wp-block-wpcomsp-dynamic-table-of-contents a[href="#${ $id }"]` | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Scope queries to this block to avoid cross‑block interference.
If multiple TOC blocks exist, global queries will cross‑talk. Scope to $tocBlock.
Apply:
-const $links = document.querySelectorAll(
- '.wp-block-wpcomsp-dynamic-table-of-contents a'
-);
+const $links = $tocBlock.querySelectorAll( 'a' );
...
-const $link = document.querySelector(
- `.wp-block-wpcomsp-dynamic-table-of-contents a[href="#${ $id }"]`
-);
+const $link = $tocBlock.querySelector( `a[href="#${ $id }"]` );
+if ( ! $link ) {
+ return;
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const $observer = new IntersectionObserver( | |
| ( entries ) => { | |
| const $links = document.querySelectorAll( | |
| '.wp-block-wpcomsp-dynamic-table-of-contents a' | |
| ); | |
| entries.forEach( ( entry ) => { | |
| const $id = entry.target.id; | |
| const $link = document.querySelector( | |
| `.wp-block-wpcomsp-dynamic-table-of-contents a[href="#${ $id }"]` | |
| ); | |
| const $observer = new IntersectionObserver( | |
| ( entries ) => { | |
| const $links = $tocBlock.querySelectorAll( 'a' ); | |
| entries.forEach( ( entry ) => { | |
| const $id = entry.target.id; | |
| const $link = $tocBlock.querySelector( `a[href="#${ $id }"]` ); | |
| if ( ! $link ) { | |
| return; | |
| } |
🤖 Prompt for AI Agents
In dynamic-table-of-contents/src/view.js around lines 26 to 36, the
IntersectionObserver callback uses global document queries which causes multiple
TOC blocks to interfere; change
document.querySelectorAll('.wp-block-wpcomsp-dynamic-table-of-contents a') to
scope to the current block (use $tocBlock.querySelectorAll(...)) and change
document.querySelector(`.wp-block-wpcomsp-dynamic-table-of-contents a[href="#${
$id }"]`) to $tocBlock.querySelector(`a[href="#${ $id }"]`) so both the links
list and per-entry link lookup are limited to the current TOC block. Ensure
selectors no longer include the global block class when scoped to $tocBlock.
What's Changed
data-heading-levelsattribute on the block wrapperHow to Test
Add the block to a post/page:
Test heading level selection:
Verify frontend functionality:
Test backward compatibility:
Edge cases to test:
Backward Compatibility
✅ All existing Dynamic Table of Contents blocks will continue to function exactly as before, with all heading levels included by default.
Summary by CodeRabbit
New Features
Documentation