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

Implement anchors across all pages #517

Closed
wants to merge 4 commits into from
Closed

Conversation

ishaanbedi
Copy link
Contributor

Motivation:

As per issue #497, implemented support for anchors against headings across all pages.

Modifications:

Implemented https://github.com/allejo/jekyll-anchor-headings (MIT Licensed) to have support for anchors.

  • Created a _includes/anchor_headings.html file which contains the file from the above-mentioned repo.
  • Modified _layouts/article.html, _layouts/default.html, _layouts/post.html files to replace the existing Liquid tag with the tag as mentioned in the repo with the anchor's site-wide configurations.
  • Modified assets/stylesheets/_screen.scss file to
    • Add scroll-behavior: smooth; property for smooth scrolling when anchors are clicked.
    • Initialize the anchor with the #anchor id to be hidden by default and only appear when hovered/focused upon.

Result:

Anchors appear when a heading is hovered, taken to the specified section when clicked upon, with the fragment name being displayed in the URL in the address bar followed by the identifier:

Screen.Recording.2024-02-06.at.8.25.31.AM.mov

Marking this as a draft PR for now!

CC: @alexandersandberg @federicobucchi @parispittman

@ishaanbedi
Copy link
Contributor Author

Hey @alexandersandberg !
I saw a similar PR #613 from @federicobucchi that has been approved recently, regarding the anchors thing.
Can you please guide me if I should go ahead with closing this PR?

@alexandersandberg
Copy link
Member

Hey @alexandersandberg ! I saw a similar PR #613 from @federicobucchi that has been approved recently, regarding the anchors thing. Can you please guide me if I should go ahead with closing this PR?

Hi @ishaanbedi, sorry for the very late response here. I'm also sorry for the confusion and lack of communication and collaboration with you.

The other PR was opened by a workgroup member who was not aware of your existing PR. We then discussed these two PRs in a meeting a concluded that your approach of adding anchors build-time instead of using JavaScript was better, but we were a bit concerned with the addition of the 3rd-party dependency vs. just writing something simple ourselves. The other PR was then updated to implement and similar solution to yours, but with a slightly simpler approach, and without a 3rd party dependency.

The right way here would have been for us to reach out to you and work with you to get your PR to a point we would have been happy with, and not implement our own solution on the side and then dismiss yours—that's not how the workgroup wants to work, because we do really appreciate and encourage community contributions. Due to some miscommunication within the workgroup this time, however, we failed on this part, and I apologize for that.

As #613 now is at a pretty far along state, I indeed would say that it's better to close this PR and try to get this other one merged instead. We would love to get your take on it though, so please review it and let us know what you think.

Lastly, thanks for working on this PR—although it was done in a different PR now, your implementation was a great source of inspiration. 🙂

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.

None yet

2 participants