default.html: remove scrollspy

kmk3 commented 3 years ago

Added on commit 36ad237 ("Add left menu bar nav with scrollspy support") and commit 6371fe0 ("use minified scrollspy.min.js on cdn").

From my testing on this site, Scrollspy is not working. The version that is currently used is loaded from a CDN as a standalone file (65 SLOC unminified)[1]. But on later Bootstrap versions, it does not seem to be possible to load Scrollspy by itself, as only the entire bootstrap(.bundle)(.min).js appears to be available (~3262-4501 SLOC unminified)[2][3][4].

Additionally, while Scrollspy is used on the navbar of Bootstrap's documentation up until the 3.x series[5][6], since version 4.0.0 (released on 2018-01-18), the component is no longer used[7][8].

Considering that: It's a minor convenience feature, it depends on JavaScript, the current stable version of the component requires loading jQuery + Popper + the entire Bootstrap framework[9] and since Bootstrap themselves stopped using it, put it to rest as well.

As a bonus, this makes jQuery no longer needed, so remove it too.

[1] https://cdnjs.cloudflare.com/ajax/libs/twitter-bootstrap/1.4.0/js/bootstrap-scrollspy.min.js [2] https://cdnjs.com/libraries/twitter-bootstrap/2.0.0 [3] https://cdnjs.com/libraries/twitter-bootstrap/4.6.0 [4] https://cdnjs.cloudflare.com/ajax/libs/twitter-bootstrap/4.6.0/js/bootstrap.bundle.min.js [5] https://getbootstrap.com/docs/versions/ [6] https://getbootstrap.com/docs/3.4/javascript/#scrollspy [7] https://getbootstrap.com/docs/4.0/components/scrollspy/ [8] https://getbootstrap.com/docs/5.0/components/scrollspy/ [9] https://getbootstrap.com/docs/4.6/getting-started/introduction/

xuhdev commented 3 years ago

Thanks for pointing this issue out. Strange, I remember it used to be working. Do you have any idea why it stopped working?

kmk3 commented 3 years ago

Thanks for pointing this issue out.

No problem.

Strange, I remember it used to be working.

Personally, I don't particularly remember the website ever having the feature, and I've known about EditorConfig for at least a few years. Though the visual effect is usually rather subtle (with good reason), so it might just be that I never paid much attention to it.

Do you have any idea why it stopped working?

Not really. Before opening the PR, I noticed that Scrollspy still works on the Bootstrap docs even on the 1.4.0 version, so out of curiosity I had tried replacing js files on the website with the same exact same ones used on the docs:

But it didn't work. I also tried the files suggested on the 4.6.0 docs (also using the new markup attributes):

But even copying and pasting the entire first example from the docs (and making the paragraphs long enough) didn't work:

https://getbootstrap.com/docs/4.6/components/scrollspy/#example-in-navbar

There were no errors in the console in any case. Admittedly, I didn't try very hard to debug it and even though I do find the feature to be a nice touch (like how it's used in the Bootstrap docs), I'm more inclined towards just dropping the includes, especially since currently they have no effect anyway.

kmk3 commented 3 years ago

(Rebased to master to fix conflict with #145)

Pittan commented 3 years ago

Let me check & review later today 🙏

kmk3 commented 3 years ago

(Rebased to master to fix conflict with #147)

kmk3 commented 3 years ago

Let me check & review later today pray

Hello, feel free to let me know if you need anything else for the review.

Pittan commented 3 years ago

@kmk3 @xuhdev Sorry for the delay, I just checked everything! As it turns out, this change seems to be fine.

This commit changed the URL format of the link, so scrollspy stopped working from this point on.

    <ol data-scrollspy="scrollspy">
-      <li><a href="#overview">What is EditorConfig?</a></li>
+      <li><a href="{{ site.baseurl }}/#overview">What is EditorConfig?</a></li>
    </ol>

To test this, I executed the code below in Chrome's developer tools. This code will fix the link and refresh scrollspy to work again.

document.querySelectorAll('[data-scrollspy] > li > a').forEach(anchorTag => {
  const currentHref = anchorTag.getAttribute('href')
  anchorTag.setAttribute('href', currentHref.replace(/\/(.*)/, '$1'))
  console.log(currentHref)
})
$('body').scrollSpy('refresh')

But this solution will cause a problem (@kmk3 made this fix and reverted at https://github.com/editorconfig/editorconfig.github.com/pull/147) so disable scrollspy for now and re-implement in the future will be the best choice.

kmk3 commented 3 years ago

@Pittan commented 5 hours ago:

@kmk3 @xuhdev Sorry for the delay, I just checked everything! As it turns out, this change seems to be fine.

This commit changed the URL format of the link, so scrollspy stopped working from this point on.
    <ol data-scrollspy="scrollspy">
-      <li><a href="#overview">What is EditorConfig?</a></li>
+      <li><a href="{{ site.baseurl }}/#overview">What is EditorConfig?</a></li>
    </ol>
To test this, I executed the code below in Chrome's developer tools. This code will fix the link and refresh scrollspy to work again.
document.querySelectorAll('[data-scrollspy] > li > a').forEach(anchorTag => {
  const currentHref = anchorTag.getAttribute('href')
  anchorTag.setAttribute('href', currentHref.replace(/\/(.*)/, '$1'))
  console.log(currentHref)
})
$('body').scrollSpy('refresh')
But this solution will cause a problem (@kmk3 made this fix and reverted at #147) so disable scrollspy for now and re-implement in the future will be the best choice.

Great analysis; I didn't know that you were going to bisect it.

I find it interesting that it was broken such a long time ago; wasn't expecting that. I did the math and apparently it has actually been broken (commit 67a887a) for longer than it had been working (commit 36ad237):

$ git show --pretty='%h %ai %s' -s 36ad237 67a887a
36ad237 2012-01-20 07:17:34 -0800 Add left menu bar nav with scrollspy support
67a887a 2016-07-13 02:23:41 +0200 Add a Jekyll-based blog (#57)

# working
$ git show --pretty='%ad' --date=unix -s 36ad237
1327072654
$ git show --pretty='%ad' --date=unix -s 67a887a
1468369421
$ expr 1468369421 - 1327072654
141296767
$ printf '141296767 / 60 / 60 / 24 / 365\n' | bc -l
4.48049108954845256215

# broken
$ expr "$(date +'%s')" - 1468369421
152731657
$ printf '152731657 / 60 / 60 / 24 / 365\n' | bc -l
4.84308907280568239472

So roughly it had worked for 4 years and ~6 months and has been broken for 4 years and ~9 months.

xuhdev commented 3 years ago

Awesome! Thanks for figuring this out, @kmk3 and @Pittan !

editorconfig / editorconfig.github.com

default.html: remove scrollspy #142