-
Notifications
You must be signed in to change notification settings - Fork 53
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
Add identifierItems support #691
base: main
Are you sure you want to change the base?
Conversation
if (item.overrideTitle) { | ||
ref.title = item.overrideTitle; | ||
} | ||
if (item.overridingTitleInlineContent) { |
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.
I think the reason that this might not be working like you expect is that the logic for overridingTitle
and overridingTitleInlineContent
lives in ContentNode
, which is not used by the TopicsLinkBlock
component to render the see-also links (it directly uses <a>
and <router-link>
components).
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.
I think Marcus is right. It would probably use the overrideTitle
as TopicsLinkBlock
already uses the title
property, but it does not know what titleInlineContent
is.
Since that property is a RenderJSON format, we would have to conditionally render the ContentNode
inside the TopicsLinkBlock
, only when titleInlineContent
is present for example. Similar to how we call renderChildren(titleInlineContent)
in ContentNode.vue
.
I am not sure how we should deal with the titleTag
in that situation either.
@@ -110,8 +110,20 @@ export default { | |||
sectionsWithTopics() { | |||
return this.sections.map(section => ({ | |||
...section, | |||
topics: section.identifiers.reduce( | |||
(list, id) => (this.references[id] ? list.concat(this.references[id]) : list), | |||
topics: section.identifierItems.reduce( |
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.
Should we worry about backwards compatibility? @mportiz08
if (this.references[item.identifier]) { | ||
const ref = this.references[item.identifier]; | ||
if (item.overrideTitle) { | ||
ref.title = item.overrideTitle; |
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.
Because JS objects are retrieved as references, this will change the source object inside the this.references
entirely, overriding the original title and content everywhere, which I think is not good.
We should create a clone of that reference, so we can safely augment things as needed.
if (item.overrideTitle) { | ||
ref.title = item.overrideTitle; | ||
} | ||
if (item.overridingTitleInlineContent) { |
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.
I think Marcus is right. It would probably use the overrideTitle
as TopicsLinkBlock
already uses the title
property, but it does not know what titleInlineContent
is.
Since that property is a RenderJSON format, we would have to conditionally render the ContentNode
inside the TopicsLinkBlock
, only when titleInlineContent
is present for example. Similar to how we call renderChildren(titleInlineContent)
in ContentNode.vue
.
I am not sure how we should deal with the titleTag
in that situation either.
Bug/issue #, if applicable:
Close swiftlang/swift-docc#480
Summary
Add identifierItems support to make see also link with different title
Dependencies
swiftlang/swift-docc#505
TODO
Due to my unfamiliarity with VueJS, I can't complete the PR on the swift-docc-render side independently.
The current behavior is not as expected.
It would be highly appreciated if you can give me some help. cc @dobromir-hristov
Input
with swiftlang/swift-docc#505
Ouput
Checklist
Make sure you check off the following items. If they cannot be completed, provide a reason.
npm test
, and it succeeded