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

Fix broken anchor links (#464, #465) #567

Merged
merged 5 commits into from
Apr 19, 2022

Conversation

valeriangalliat
Copy link
Member

Rebased version of #465 with addressed PR comments.

Fixes #464, closes #465.

Copy link

@d13 d13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was able to use the newly added group property by adding the following {{group_name}} code to see.html.njk in sassdoc-theme-default:

    {% set group_name = '' %}
    {% if see.group %}
      {% set group_name = see.group + '-' %}
    {% endif %}

    <li><span class="item__cross-type">[{{ see.context.type }}]</span> <a href="#{{ group_name }}{{ see.context.type }}-{{ see.context.name }}"><code>{{ see.context.name | unescape }}</code></a></li>

That being said, its highly likely to give the incorrect group if there's more than on context with the same name. I commented on the line that does the lookup, which is an existing issue prior to this change.

@@ -14,6 +14,7 @@ export default function alias (env) {

let alias = item.alias
let name = item.context.name
let aliasGroup = item.group

let aliasedItem = Array.find(data, i => i.context.name === alias)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This lookup finds the first instance of the alias name, which could be incorrect if there's more than one context with the same name.

@valeriangalliat
Copy link
Member Author

@d13 thanks for reviewing!

So my understanding is that this solution is not perfect but still yields better results than if we don't land it. Also it needs extra theme code to be useful.

What I can do is:

  • Add that code of yours to sassdoc-theme-default and release a new patch version with it (feel free to open a PR there if you want the commit attribution ☺️)
  • Merge this PR as is and release a new patch version

Does that sounds good?


As for the alias issue when multiple contexts share the same name, I'm not sure from the top of my head how that can be fixed (haven't worked with SassDoc in the past... many years) but I'll definitely accept a PR for it!

@kdaulton-chwy
Copy link

kdaulton-chwy commented Apr 19, 2022

@valeriangalliat thanks!

Yes, I agree this is an improvement!

I'll definitely put up the PR for sassdoc-theme-default.

I'll also put some time into how to solve the alias with same name issue.

(sorry for the confusion in users, just realized i'm commenting from my work account)

@valeriangalliat valeriangalliat merged commit 881a89d into master Apr 19, 2022
@valeriangalliat valeriangalliat deleted the hotfix/464-broken-anchor-links branch April 19, 2022 15:21
@valeriangalliat
Copy link
Member Author

Great! Thanks for the PR on sassdoc-theme-default

This is released with sassdoc@2.7.4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

#anchor links to references items are ungrouped for @see and @alias annotations
4 participants