backdrop-contrib / tocbot

Builds a table of contents (TOC) from content headings.
GNU General Public License v2.0
2 stars 2 forks source link

Long headings overflow when switch to fixed #42

Closed yorkshire-pudding closed 2 years ago

yorkshire-pudding commented 2 years ago

Long headings overflow from the sidebar when switch into fixed mode and it looks a bit odd and untidy. I can't think of an easy way to contain this as I guess when it switches to fix mode it loses reference to the containing block? It does seem to wrap at the page boundary so it's not a deal breaker on a right sidebar, however, on a left sidebar if overflows the text, which is a problem.

If anyone can advise of CSS that might work to contain it, or if it can be fixed in the module that would be great.

olafgrabienski commented 2 years ago

Thanks! I can confirm the issue using a layout with left sidebar, e.g. the Moscone layout: as soon as .js-toc-block gets the class .is-position-fixed, it can get wider than the sidebar and overlap with the adjacent body text.

I'm not sure about the bug status. While the HTML output on the Tocbot demo site is a bit different from the Backdrop implementation, the demo site element in question has a width definition: .toc { width: 280px; }.

I guess, the module for Backdrop shouldn't provide a strict width definition, it should be dynamic, or at least responsive. Regarding a dynamic value, the max-width of .js-toc-block.is-position-fixed could be set via JavaScript to the Tocbot block width or the sidebar width. What do you think about this approach? (I'd need some help to implement it, due to missing JS skills.)

A responsive approach which works for core Layouts could be something like this:

@media (min-width: 48em) {
  .js-toc-block.is-position-fixed {
    max-width: 150px;
  }
}
@media (min-width: 62em) {
  .js-toc-block.is-position-fixed {
    max-width: 210px;
  }
}

Which approach would you prefer?


edited the class name: was .is-positioned-fixed, corrected to .is-position-fixed

yorkshire-pudding commented 2 years ago

@olafgrabienski I think perhaps go for simple CSS responsive approach to start with; I'm not sure my JS skills are up to it either. With the example CSS above, should the 150px one be max-width: 48em ?

Happy to re-classify as enhancement if you prefer, but I thought overlapping content on core layouts would count as a bug.

olafgrabienski commented 2 years ago

@yorkshire-pudding Thanks for your feedback and for the hint re the CSS example. In the second part with 210px I wanted to use 62em (now edited and corrected).

Agreed, let's start with the responsive approach. If you don't beat me to it, I can suggest the CSS addition in the next days.

Re classification: no problem, let's stick with bug.

olafgrabienski commented 2 years ago

The following CSS addition works for me with the core layouts containing one or two sidebars (Harris, Moscone, Moscone Flipped, Simmons, Taylor, Taylor Flipped):

@media (min-width: 48em) {
  .js-toc-block.is-position-fixed {
    max-width: 150px;
  }
}
@media (min-width: 62em) {
  .js-toc-block.is-position-fixed {
    max-width: 210px;
  }
}
@media (min-width: 75em) {
  .js-toc-block.is-position-fixed {
    max-width: 259px;
  }
}

I'll provide a PR soon. One question: Does it make more sense to use .is-position-fixed, without combining it with js-toc-block? At first sight, .is-position-fixed looks very general to me, it could come from every other module than Tocbot. On the other hand, so far the class has been used without combination, and it may be better to follow that practice, at least for the time being. @yorkshire-pudding What do you think?

yorkshire-pudding commented 2 years ago

@olafgrabienski - tested and works well for me. I agree that it seems quite general and might run the risk that another module picks the same class and then has unforeseen consequences. Might also want to move the top: 80px; to be under .js-toc-block.is-position-fixed

olafgrabienski commented 2 years ago

Might also want to move the top: 80px; to be under .js-toc-block.is-position-fixed

Done, wanna have a look before I merge the PR? (I've also added some missing spaces.)

yorkshire-pudding commented 2 years ago

@olafgrabienski - looks good.