Skip to content
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

Table of Contents Snippet exclusion system #3261

Open
wants to merge 58 commits into
base: master
Choose a base branch
from

Conversation

dbolack-ab
Copy link
Collaborator

@dbolack-ab dbolack-ab commented Jan 20, 2024

Based on @ericscheid's suggestion.

This modifies the Table of Contents snippet to ignore headers that have an active CSS variable --TOC with a value of exclude. To facilitate backward compatibility, CSS classes have been added or altered to have this value set along with the monster class in the 5ePHB theme. Additional classes have been added for toggling on and off exclusions.

To Be Determined:

Which of this CSS work should relocate to the Blank class.

either I am building the lookup incorrectly or Chrome is not letting me
see variables via computedStyles.
@dbolack-ab dbolack-ab added the 🔍 R0 - Needs first review 👀 PR ready but has not been reviewed label Jan 20, 2024
@ericscheid
Copy link
Collaborator

This js works:

 var iframe = document.getElementById('BrewRenderer');
 var iframeDocument = iframe.contentDocument || iframe.contentWindow.document;
 var headings = iframeDocument.querySelectorAll('h1, h2, h3, h4, h5, h6');

 headings.forEach(function (heading) {
   let text = heading.innerText;
   let page = heading.closest('.page,.phb').id;
   let excluded = getComputedStyle(heading).getPropertyValue("--TOC");
   console.log({page, text, excluded});
   // add to the snippet output 
   }

@dbolack-ab dbolack-ab self-assigned this Jan 21, 2024
@dbolack-ab dbolack-ab changed the title CSS based ToC exclusion system for Issue 2994 Table of Contents Snippet exclusion system Jan 24, 2024
@calculuschild
Copy link
Member

Getting an error trying this on the home page:

image
image

@calculuschild
Copy link
Member

calculuschild commented Jan 26, 2024

@ericscheid, @5e-Cleric , @Gazook89 , @G-Ambatte Is the current behavior acceptable to you? Default exclusions with manual inclusion and exclusion tags via CSS? If the behavior is agreed upon I will just check over for code quality and bugs.

@calculuschild calculuschild added 🔍 R3 - Reviewed - Awaiting Fixes 🔧 PR is okayed but needs fixes before merging and removed 🔍 R0 - Needs first review 👀 PR ready but has not been reviewed labels Jan 26, 2024
@ericscheid
Copy link
Collaborator

Getting an error trying this on the home page:

  1. Firstly, why is there a heading element inside a <div class="page"> where the latter doesn't have an id attribute? What use case is causing this error??

  2. amend to use optional chaining .. and a null will be fed to parseInt instead of throwing an access error.
    let onPage = parseInt(heading.closest('.page,.phb').id?.replace(/^p/,′);

  3. amend the line a bit further where onPage is checked to instead test against NaN (which parseInt(null) produces)
    if(!onPage.isNaN(myValue)) {

@ericscheid
Copy link
Collaborator

@ericscheid, @5e-Cleric , @Gazook89 , @G-Ambatte Is the current behavior acceptable to you? Default exclusions with manual inclusion and exclusion tags via CSS? If the behavior is agreed upon I will just check over for code quality and bugs.

The snippet js starts with the assumption of h1,h2,h3,h4,h5,h6 (currently just h1..h4) and then the general exclusion rules are applied. The h5 and h6 have been added to the current ToC snippet, and then excluded in the css, so as to support any author that might want to later include those levels of headings.

The use of exclusion rules is short and simple to modify and covers the desired exclusions (h3 and h4 inside .monster, various cover pages, h5, h6). There's an argument for also excluding .monster h2 or simply .monster to mimic the base PHB theme. I would lean towards current THB compatibility than militant PHB mimicry, mostly because it's easier to understand what needs to change in the css exclusion rules (unlike if one needed to modify .monster {--TOC: exclude} to instead only exclude h3 and h4 but to also now include h2).

The philosophy of using inclusion tags (i.e. assert via manually editing brew.text in all the curly-blocks) is fraught with tedium. There is a usecase for a generic .addToC {--TOC: include} rule, both for the occasional usage for exceptions, but also to demonstrate what value other than exclude can be used. The rule would need to have stronger specificity though, or use !important.

@dbolack-ab
Copy link
Collaborator Author

  1. Firstly, why is there a heading element inside a <div class="page"> where the latter doesn't have an id attribute? What use case is causing this error??

My guess would be heading.closest('.page,.phb') - that the 5ePHB theme isn't being used.

@dbolack-ab
Copy link
Collaborator Author

dbolack-ab commented Jan 30, 2024

The use of exclusion rules is short and simple to modify and covers the desired exclusions (h3 and h4 inside .monster, various cover pages, h5, h6). There's an argument for also excluding .monster h2 or simply .monster to mimic the base PHB theme. I would lean towards current THB compatibility than militant PHB mimicry, mostly because it's easier to understand what needs to change in the css exclusion rules (unlike if one needed to modify .monster {--TOC: exclude} to instead only exclude h3 and h4 but to also now include h2).

This particular implementation has no inclusions for .monster based on the described problem. Should this be revised to include h1 and h2 as it does now? I think since this impacts a snippet it is far more permissible to behave differently.

@5e-Cleric 5e-Cleric removed the 🔍 R3 - Reviewed - Awaiting Fixes 🔧 PR is okayed but needs fixes before merging label May 28, 2024
@calculuschild
Copy link
Member

calculuschild commented May 28, 2024

Two more things to tweak:

  • partCover is still including the H2 header. The only thing on a partCover page that should appear in the ToC is the H1 header: {Part X)
  • In general, H3's should appear in the ToC with no #### header marker:
# Test h1

## Test h2

### Testy test h3

becomes:

- ### [{{ Test h1}}{{ 7}}](#p7)
  - #### [{{ Test h2}}{{ 7}}](#p7)
    - [{{ Testy test h3}}{{ 7}}](#p7)   <-- No ####

This PR has changed H3's to appear as - #### [{{ Testy test h3}}{{ 8}}](#p8) . Removing the #### will get us back to the current behavior for people who are just using H1-H3

I don't think there is any defined behavior for what an H4-H6 should look like in the ToC, but they shouldn't be "bigger" than an H3, which is plain text. Perhaps after H3 just continue indenting for H4-H6 with no further styling change.


Perhaps for a future PR, we may want to look at some of the consequences of a partCover. It seems it becomes the highest level ToC entry at that point and everything below gets bumped down a level, but until the first partCover, just H1 is the highest level.

@5e-Cleric 5e-Cleric added 🔍 R3 - Reviewed - Awaiting Fixes 🔧 PR is okayed but needs fixes before merging and removed 🔍 R4 - Reviewed - Fixed! ⭐ PR review comments addressed labels May 28, 2024
This fixes an error in the recusion that was failing to add children under existing parents.
Index generation now does not overindent when levels are skipped.
PartCover less code as suggesred by CC.
@dbolack-ab dbolack-ab added the 🔍 R4 - Reviewed - Fixed! ⭐ PR review comments addressed label Jun 1, 2024
@5e-Cleric
Copy link
Member

to test this are we supposed to use the first option only, or any?
image

@5e-Cleric
Copy link
Member

Not sure if this has been talked, is the type of element in the ToC set via the type of header it references, or by the position in the ToC? i have an h1 followed by h3, and the h3 gets the same styling as an h2 would.

@dbolack-ab
Copy link
Collaborator Author

Not sure if this has been talked, is the type of element in the ToC set via the type of header it references, or by the position in the ToC? i have an h1 followed by h3, and the h3 gets the same styling as an h2 would.

This was a side effect of the "over-indention protection. The current code ties the level of indention to whether or not there is H-styling on the entry.

If this is not desired ( An H1 followed by an H6 ) adjustments will need to be made.

@dbolack-ab
Copy link
Collaborator Author

Not sure if this has been talked, is the type of element in the ToC set via the type of header it references, or by the position in the ToC? i have an h1 followed by h3, and the h3 gets the same styling as an h2 would.

By the position.

@dbolack-ab
Copy link
Collaborator Author

to test this are we supposed to use the first option only, or any? image

THat is correct. How can we make this more obvious?

@5e-Cleric
Copy link
Member

to test this are we supposed to use the first option only, or any? image

THat is correct. How can we make this more obvious?

I don't understand this answer, which is it? The first option, or the latter ones??

@dbolack-ab
Copy link
Collaborator Author

to test this are we supposed to use the first option only, or any? image

THat is correct. How can we make this more obvious?

I don't understand this answer, which is it? The first option, or the latter ones??

Oh! I misread The first one. The latter ones drop the "add this level to the ToC" tags.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔍 R3 - Reviewed - Awaiting Fixes 🔧 PR is okayed but needs fixes before merging 🔍 R4 - Reviewed - Fixed! ⭐ PR review comments addressed
Projects
Status: Pushing to Finish
Development

Successfully merging this pull request may close these issues.

None yet

6 participants