-
Notifications
You must be signed in to change notification settings - Fork 6
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
Starting with h3 instead of h2 breaks the page #10
Comments
I believe i'm having the same issue as @tisonkelley. Steps to reproduce:
When I do that I get an "Undefined array key -1" If I start with an H2, then an H3 works just fine, but if I use an H1 I get same error. So issue seems to be going up a level from the original value. |
Thanks @nvkodde, I can reproduce the error now and am working on a fix probably next week. 🙏🏻 |
So, after digging into the problem a bit more, I realized, it isn't such an easy fix. The reason is, that this addon tries to build a nested tree out of the headings you provide. Therefore, the data has to be a valid tree, which has to have a proper starting point. Imagine the following: <h2>Level 2</h2>
<h3>Level 3</h3>
<h4>Level 4</h3>
<h1>Level 1</h1> In this example, when the addon would build a representative nested tree it would look like this:
Which wouldn't be in the correct order anymore, as the highest level headline would be the first one as opposed for it being the last one in the provided markup. To solve this, the algorithm tries to build the tree from top to bottom, preserving the original order of headlines. Generally, it does a solid job, when the levels differ as long as there is a clear path it can follow. In the provided example, the last level would be the root level, even though it is the last one, resulting in the reposted error, as it looks for a higher level headline on the first one. The question is now, how this addon should handle such a situation.
In my opinion, the 2nd would be the most easy to implement. Nr. 3 would be the one that could introduce a lot of unexpected behavior. |
Not sure what is meant in option 3 in regards to resetting the tree, there's probably a balance on developing something that doesn't create too many edge cases. Generally, the h1 example I would assume is a semantic issue, as you'd expect 1 per page and likely not covered by ToC anyways as it's the title of the article. For the other levels, it seems to be more common to go back/forth, pulled this from Mozilla docs (look at nesting) https://developer.mozilla.org/en-US/docs/Web/HTML/Element/Heading_Elements. So not resetting overall but just ordering the elements as you progress down the page. |
Sure, that's the general aim for this addon. But right now it has its troubles with semantic inconsistencies. A solution could be, to "reset" the tree. Which is probably a confusing term for dynamically adjusting the levels.
So, as soon as a heading level is higher than the root level, it creates a new root level. For this, the addon needs to be rewritten at a few places, but it would make sense to handle such cases properly. |
@nvkodde I pushed a first draft for it into the assigned branch: feature/dynamic-nesting |
I'm also running into this problem with my clients. I won't pretend I have read all of the above and fully understand the problem, but I can say, that erroring out if clients mess up the order of headings is definitely not ideal, as this is something that will likely happen over and over again. I would much prefer something more graceful, like flattening everything without breaking the page. Funny enough for a while I had though it "fixed" by adding |
Alright. I'll come up with something. I understand, coming from a clients-perspective, breaking the page because of a semantic error isn't ideal. However, as the error itself is caused by "wrong" semantics due to user input, It will be highly opinionated. |
@tisonkelley reported in #7
The text was updated successfully, but these errors were encountered: