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 for TaskGroupRenderSection #505

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 Mar 12, 2023

Bug/issue #, if applicable:

Close #480

Summary

Update identifiers to identifierItems in TaskGroupRenderSection

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

Before the PR

JSON output:

"seeAlsoSections": [
    {
        "title": "Related Documentation",
	    "identifiers": [
	      "https://example.com",
	      "https://example.com",
	      "https://example3.com",
	      "https://example3.com"
	    ]
    }
]

swift-docc-render:
image

After the PR

JSON output:

"seeAlsoSections": [
  {
    "title": "Related Documentation",
    "identifiers": [
      "https://example.com",
      "https://example.com",
      "https://example3.com",
      "https://example3.com"
    ],
    "identifierItems": [
      {
        "identifier": "https://example.com"
      },
      {
        "overrideTitle": "Name 2",
        "overridingTitleInlineContent": [
          {
            "type": "text",
            "text": "Name 2"
          }
        ],
        "identifier": "https://example.com"
      },
      {
        "identifier": "https://example3.com"
      },
      {
        "overrideTitle": "this great link ",
        "overridingTitleInlineContent": [
          {
            "type": "text",
            "text": "this great link "
          },
          {
            "type": "image",
            "identifier": "https://picsum.photos/200"
          }
        ],
        "identifier": "https://example3.com"
      }
    ]
  }
]

swift-docc-render:

Dependencies

Swift-DocC: #376 (Merged)
Swift-DocC-Render: swiftlang/swift-docc-render#691

Testing

[TODO]

Checklist

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

  • Added tests
  • Ran the ./bin/test script and it succeeded
  • Updated documentation if necessary

@franklinsch
Copy link
Contributor

This approach makes sense to me. It would be good to align with the approach we've taken with the overridingTitleInlineContent parameter of RenderInlineContent.reference so that we support formatting in the title for extra flexibility.

@Kyle-Ye Kyle-Ye marked this pull request as ready for review June 7, 2023 15:19
@Kyle-Ye
Copy link
Contributor Author

Kyle-Ye commented Jun 7, 2023

This approach makes sense to me. It would be good to align with the approach we've taken with the overridingTitleInlineContent parameter of RenderInlineContent.reference so that we support formatting in the title for extra flexibility.

Update the swift-docc side implementation with your suggestion.

And the swift-docc-render PR is half-ready.

@mportiz08
Copy link
Contributor

I have a minor concern about the proposed Render JSON changes here. I like that the new identifierItems section was added as a way to maintain backwards compatibility for the new feature as opposed to modifying the existing identifiers array structure.

However, it seems that the new array will always be presented, even in the case where it isn't needed for the more common scenario where none of the links have custom titles. I'm slightly worried that this will add to the size of large libraries of content since the object with the duplicated identifier will be added for each see-also item.

Is there a way to make this an optional field that only gets included in Render JSON when it's needed for custom titles?

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