-
Notifications
You must be signed in to change notification settings - Fork 60
Inactivate known external links #878
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
Inactivate known external links #878
Conversation
Instead of passing a new boolean flag to indicate which refs should be included or not, just strip out the URL data for refs that are not part of included archives and should not be linked to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much for working on this approach!
I did a quick pass in docc-render and found that Mention.vue
also uses the Reference
component and has a required url
prop. Should we also make that prop optional? Although it doesn't really make sense to render a title without a link in this section. @d-ronnqvist What's the plan for the "mentioned in" section for multi-target docs?
src/mixins/referencesProvider.js
Outdated
} | ||
|
||
return includedArchiveIdentifiers.some(archiveId => ( | ||
id?.startsWith(`doc://${archiveId}`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Continuing the discussion we started in the original PR: #873 (comment)
I think we need to check if the identifier starts with doc://${id}/documentation. Just to make sure that the id matches exactly. (For example, Foo and FooBar)
That would only work for documentation pages and not tutorial pages. We could make this more strict, but I think checking for the special scheme and bundle ID should be sufficient (@d-ronnqvist correct me if I'm wrong)
You are right, that wouldn't work for tutorial pages. But I still think this wouldn't work if the frameworks in question are named: 'Foo' and 'FooCore'.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I think we can change this to .startsWith(`doc://${archiveId}/`)
instead (just adding the trailing slash) to fix this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you also add a test for this edge case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed with 6edce2b and included a test case that fails without the trailing slash
FIY: this doesn't depend on swiftlang/swift-docc-plugin#84. If anything it depends on swiftlang/swift-docc#821 which was merged in February. |
Thanks, updated |
@swift-ci test |
Inactivate known external links (swiftlang#878) rdar://118834404 --------- Co-authored-by: Hanqing Huang <[email protected]>
Bug/issue #, if applicable: 118834404
Summary
This adds logic to inactivate links to pages from other targets unless they have been included in the same combined archive.
Dependencies
swiftlang/swift-docc#821
Testing
You can use the archives from this fixture for the convenience of testing purposes:
archives.zip
Steps:
VUE_APP_DEV_SERVER_PROXY=/path/to/Combined\ ExternalLinks\ Documentation.doccarchive npm run serve
and open http://localhost:8080/documentation/outer/outerclass/dosomething(with:)VUE_APP_DEV_SERVER_PROXY=/path/to/Outer.doccarchive npm run serve
and open http://localhost:8080/documentation/outer/outerclass/dosomething(with:)Known issues:
Due to the way that the Render JSON and Navigator JSON are loaded independently by the renderer, it's possible that the links in step 4 may be initially linked for a very brief moment until the navigator has finished loading. I've discussed this with @d-ronnqvist, and this limitation is acceptable for now given the way that things are structured.
Checklist
Make sure you check off the following items. If they cannot be completed, provide a reason.
npm test
, and it succeeded