Skip to content

Conversation

bswck
Copy link
Member

@bswck bswck commented Jul 13, 2025

Closes #671

@bswck bswck requested a review from Copilot July 13, 2025 16:20
Copilot

This comment was marked as outdated.

@bswck
Copy link
Member Author

bswck commented Jul 13, 2025

Great, tests broke with default skip_inventory=True as expected :-)

@bswck
Copy link
Member Author

bswck commented Jul 13, 2025

The initial suggestion was that we would use a data-inventory attribute and decide about registering based on that.

However, that has approach 2 problems:

  • It doesn't conform to the HTML standard on boolean attributes (https://html.spec.whatwg.org/#boolean-attributes)
  • It creates more friction, as now all elements that are automatically registered (which is the vast majority) will have data-inventory="true" and _process_headings would have to treat (1) no presence of data-inventory and (2) presence with a truthy value (which requires a new convention, because it's string checking; like: is data-skip-inventory="n" a truthy or falsey value?) as the same flag. I don't like that.

Therefore, I suggest that we only add data-skip-inventory to all excluded elements. It will treat any presence of data-skip-inventory as the intent to skip the inventory, so all elements from

<h1 data-skip-inventory></h1>
<h1 data-skip-inventory=""></h1>
<h1 data-skip-inventory="true"></h1>
<h1 data-skip-inventory="false"></h1>  <!-- sic -->

would be treated as excluded from registration in the inventory, and only elements without the presence of data-skip-inventory would be registered.

@bswck bswck linked an issue Jul 13, 2025 that may be closed by this pull request
@bswck bswck changed the title feat: Add flag for elements to skip inventory feat: Add data-skip-inventory boolean attribute for elements to skip local inventory Jul 13, 2025
@bswck
Copy link
Member Author

bswck commented Jul 13, 2025

@pawamoy
Copy link
Member

pawamoy commented Jul 14, 2025

Thanks for the analysis and taking the right direction! Also, clever example in the docs!

@bswck bswck marked this pull request as ready for review July 16, 2025 06:16
@bswck bswck requested a review from pawamoy July 16, 2025 06:16
bswck and others added 2 commits July 21, 2025 16:41
Co-authored-by: Timothée Mazzucotelli <dev@pawamoy.fr>
Co-authored-by: Timothée Mazzucotelli <dev@pawamoy.fr>
Copy link
Member

@pawamoy pawamoy left a comment

Choose a reason for hiding this comment

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

OK LGTM, thanks a lot!

@pawamoy pawamoy merged commit f856160 into main Jul 21, 2025
53 checks passed
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.

feature: No-inventory YAML option
2 participants