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

Add ability for user to set depth limit by adding data-levels="#" #36

Closed
wants to merge 2 commits into from

Conversation

kudaba
Copy link

@kudaba kudaba commented Jun 6, 2017

Fix for #27.

Note: I'm not sure if there's a better way to do the css than just copy pasting up to 6 levels of depth.

Add ability for user to set depth limit by adding data-levels="#" to the root nav element.

  • The number of levels caps at maximum of 6 when using h1 (5 when using h2 etc)
  • The algorithm can only increment one level at a time, but can drop to lowest available level

…the root nav element.

* The number of levels caps at maximum of 6 when using h1 (5 when using h2 etc)
* The algorithm can only increment one level at a time, but can drop to lowest available level
@MathieuDuponchelle
Copy link

Erm, I'm sorry, had cloned the repo some time ago and just got to doing the same thing as you did before seeing this PR, I just opened #37 :(

@kudaba
Copy link
Author

kudaba commented Jun 19, 2017

@MathieuDuponchelle no problem, your css was way better than mine, though I have the bonus of unit tests and bounds checking the depth. I stole your css but fixed the minor issue of the text shifting 1 pixel when hovering. I also changed the font % since at 6 levels it was making the text very small.

@MathieuDuponchelle
Copy link

MathieuDuponchelle commented Jun 19, 2017

@kudaba , the text shifting was actually intentional :)

As for the size percentage, I must say I didn't test beyond 4 levels, not sure whether anything beyond that will often be used :)

Curious, how does your PR handle:

## H2

#### H4

### H3

## H2

?

@MathieuDuponchelle
Copy link

As for bounds checking, can you explain the problem with my PR ? afaict ther's no issue with specifying a too large depth, t's just useless ?

@kudaba
Copy link
Author

kudaba commented Jun 19, 2017

  1. It explicitly ignores level jumps greater than 1, mainly since it would take a lot more code to deal with in a nice way and I wanted to keep it simple. Collapsing multiple levels (h1, h2, h3, h1) is ok though.
  2. Generally there's not much of an issue, there might be a small performance hit if someone puts depth 1000000, but that would just be silly. It's mostly just to keeps things safe.

@MathieuDuponchelle
Copy link

@kudaba , then I would advise using my PR, at least as the base, it does handle any sort of level jumps, which was mandatory for me.

@kudaba
Copy link
Author

kudaba commented Jun 19, 2017

I gave it a try and there's two issues I ran into:

  1. There's no way to specify depth in the html, only if you use the javascript directly
  2. The behavior when skipping a level is a bit weird, the item only shows up in the table when scrolling around it, so you can't navigate to the item.

@MathieuDuponchelle
Copy link

There's no way to specify depth in the html, only if you use the javascript directly

fixed

The behavior when skipping a level is a bit weird, the item only shows up in the table when scrolling around it, so you can't navigate to the item.

Fixed as well, good catch, I was wondering what was bugging me when testing it

@kudaba
Copy link
Author

kudaba commented Jun 20, 2017

Instead of this change I made a pull request to your branch with a few small fixes and my unit tests. closing this one and will go with yours instead since the base implementation is simpler and more robust.

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.

2 participants