Skip to content

Hotfix/464 broken anchor links - #465

Closed
pete-hotchkiss wants to merge 3 commits into
SassDoc:masterfrom
pete-hotchkiss:hotfix/464-broken-anchor-links
Closed

Hotfix/464 broken anchor links#465
pete-hotchkiss wants to merge 3 commits into
SassDoc:masterfrom
pete-hotchkiss:hotfix/464-broken-anchor-links

Conversation

@pete-hotchkiss

Copy link
Copy Markdown
Contributor

has fixes for broken anchor references when annotation link to items in other @group's

@coveralls

coveralls commented Aug 29, 2016

Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.02%) to 94.634% when pulling ca59cdf on pete-hotchkiss:hotfix/464-broken-anchor-links into 2bd49f9 on SassDoc:master.

@ghost

ghost commented Oct 19, 2016

Copy link
Copy Markdown

What needs to happen to merge this pull?

@KittyGiraudel

Copy link
Copy Markdown
Member

A review from maintainers. :D

Ping @SassDoc/owners.

@pascalduez

pascalduez commented Oct 19, 2016

Copy link
Copy Markdown
Member

I tried to wrap my head around this some time ago, and was not convinced this is the right way, there's a missing brick on why it is this way.
Will give it another look.

@pete-hotchkiss

Copy link
Copy Markdown
Contributor Author

been a few months since I wrote this code, but I recall being aware my solution was somewhat 'baked' - i.e effectivly baking a new property into the annotation object is far from elegant. Would be more preferable to have a reference to the parent scope so we could access the grouping property directly rather than adding/removing as we go.

I was also aware I didn't want to completely break existing theme templates...

My stuff is ugly, but it works - and it got my documentation working with a locally patched version of the library.

@KittyGiraudel

Copy link
Copy Markdown
Member

Hey I’m kind of busy right now, but this is on my radar. I’ll try to check this next week.

aliasedItem.aliased = [];
}

if (!Array.isArray(aliasedItem.aliasedgroup)) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We should probably use aliasedGroup to respect camelCase conventions?

@KittyGiraudel

Copy link
Copy Markdown
Member

@SassDoc/owners The code looks simply enough to me. Can someone review it as well? I might lack a bit of context on this part of the code base.

@valeriangalliat valeriangalliat left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

2 small comments but otherwise LGTM, thanks 👍

Can't be sure it's the perfect way or what but the code is pretty simple, so if it works that's sufficient for me. :P


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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

let

@pete-hotchkiss

pete-hotchkiss commented Oct 26, 2016

Copy link
Copy Markdown
Contributor Author

I'm out and about today, but I'll make the changes to the camelCasing this
evening and update the PR.

@LeaveAirykson

Copy link
Copy Markdown

Is there any progress on this request? the problem still exists..

@pascalduez

Copy link
Copy Markdown
Member

There's a few changes requested though.

@LeaveAirykson

Copy link
Copy Markdown

I understand that there are many requests and i didn't mean to be impatient. It's just that this pull request is open for almost 2 years now and no further progress was reported. just wanted to know if there are any news because we handle large projects with sassdoc and grouping is necessary to keep our documentary organized and this issue makes the linking kind of useless.

@d13

d13 commented Apr 18, 2022

Copy link
Copy Markdown

I understand that there are many requests and i didn't mean to be impatient. It's just that this pull request is open for almost 2 years now and no further progress was reported. just wanted to know if there are any news because we handle large projects with sassdoc and grouping is necessary to keep our documentary organized and this issue makes the linking kind of useless.

Same for me, I heavily rely on groups in a very large codebase.

What can I do to help get this over the finish line? Is the hold-up just camel-casing and changing a var to let?

@KittyGiraudel

Copy link
Copy Markdown
Member

Hello there. 👋

I am no longer maintaining SassDoc, nor do I have time to invest in that project anymore. Unfortunately, I don’t think we have dedicated maintainers to take over, so the project is kind of frozen in time and place. I‘m sorry about this.

@valeriangalliat

Copy link
Copy Markdown
Member

Hey! On top of the PR comments, a quick rebasing / conflicts fixing needed to be done. I've pushed an up-to-date version of this PR in #567.

@d13 any chance you can checkout the code from #567 and run it against your codebase to confirm this fixes the issue for you as well? Something like cd node_modules; rm -rf sassdoc; git clone --branch hotfix/464-broken-anchor-links https://github.com/SassDoc/sassdoc; cd sassdoc; yarn; make should do.

Since this was opened as a hotfix and I approved it back then without any concern about breaking changes, I believe it would make sense to release this with a patch version. Do that sound reasonable @KittyGiraudel?

Cheers!

@KittyGiraudel

Copy link
Copy Markdown
Member

Definitely. Happy to release a hotfix once it‘s merged.

valeriangalliat added a commit that referenced this pull request Apr 19, 2022
@valeriangalliat

Copy link
Copy Markdown
Member

Released with sassdoc@2.7.4 and sassdoc-theme-default@2.8.5!

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.

7 participants