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

Add identifierItems support #691

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

Conversation

Kyle-Ye
Copy link
Contributor

@Kyle-Ye Kyle-Ye commented Jun 7, 2023

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

## Overview

For more information see the [specification](https://example.com).

## See Also
- [Name 1](https://example.com)

- [Name 2](https://example.com)

- [Name 3](https://example3.com)

- [this great link ![random image](https://picsum.photos/200)](https://example3.com)

with swiftlang/swift-docc#505

Ouput

image

Checklist

Make sure you check off the following items. If they cannot be completed, provide a reason.

  • Added tests
  • Ran npm test, and it succeeded
  • Updated documentation if necessary

if (item.overrideTitle) {
ref.title = item.overrideTitle;
}
if (item.overridingTitleInlineContent) {
Copy link
Contributor

@mportiz08 mportiz08 Jun 9, 2023

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.

@@ -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

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.overrideTitle) {
ref.title = item.overrideTitle;
}
if (item.overridingTitleInlineContent) {
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.

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

Successfully merging this pull request may close these issues.

Impossible to put same link in body and see also with different title
3 participants