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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add identifierItems support #691

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions src/components/DocumentationTopic/TopicsTable.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Copy link
Contributor

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

(list, item) => {
if (this.references[item.identifier]) {
const ref = this.references[item.identifier];
if (item.overrideTitle) {
ref.title = item.overrideTitle;
Copy link
Contributor

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.overridingTitleInlineContent) {
Copy link
Contributor

@mportiz08 mportiz08 Jun 9, 2023 •
edited
Loading

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).

Copy link
Contributor

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.

ref.titleInlineContent = item.overridingTitleInlineContent;
}
return list.concat(ref);
}
return list;
},
[],
),
}));
Expand Down