Skip to content

Add page image directive #367

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

Merged

Conversation

ethan-kusters
Copy link
Contributor

@ethan-kusters ethan-kusters commented Aug 26, 2022

Bug/issue #, if applicable: rdar://97739580

Summary

Add support for an @PageImage directive that allows for customizing a given page's card or icon image.

Dependencies

Testing

  1. Build a copy of Swift-DocC-Render from Support custom page icons and image overrides swift-docc-render#417
  2. Add an SVG with an ID to a DocC catalog: plus.svg
  3. Add a page image metadata directive to a documentation extension file or article:
@Metadata {
  @PageImage(source: "plus", purpose: icon)
}
  1. Confirm that the icon appears in the navigator and hero. Confirm that there are references to the icon on any page that references the customized page, the page itself, and the index.json.

Screenshot 2022-09-15 at 4 50 06 PM

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

@ethan-kusters ethan-kusters force-pushed the add-page-image-directive branch 4 times, most recently from 08754b5 to 27c7011 Compare September 1, 2022 15:21
@ethan-kusters
Copy link
Contributor Author

@swift-ci please test

1 similar comment
@ethan-kusters
Copy link
Contributor Author

@swift-ci please test

@ethan-kusters ethan-kusters force-pushed the add-page-image-directive branch 2 times, most recently from b16e565 to 2382375 Compare September 15, 2022 15:38
/// A basic XML parser that extracts the first `id` attribute found in the given SVG.
///
/// This is a single-purpose tool and should not be used for general-purpose SVG parsing.
enum SVGIDExtractor {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In order to fully integrate with Swift-DocC-Render's theming system for custom icons, we need to be able to provide the id of any custom SVGs we're using as icons.

Asking the user to provide these would be unfortunate since it asks them to have a greater understanding of SVGs than they might have. So instead, this is a very basic SVG parser that just extracts the first id tag it finds. We're still asking the user to provide a valid SVG, but assuming they do provide a valid one, this will pull out the ID that DocC-Render needs.

Copy link
Contributor

@dobromir-hristov dobromir-hristov Sep 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should just make sure to point out in the docs that the <svg element of the icon needs some id. They can add with any code/text editor

Copy link
Contributor Author

@ethan-kusters ethan-kusters Sep 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah definitely. Having this well documented will be really important.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we still need a warning when there's no ID?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, what will happen if a custom SVG icon doesn't have an ID?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rdar://99205219 tracks supporting rendering SVGs without IDs and other image types PNGs. SVGs with IDs will always be the best option since it allows DocC-Render to apply color tint and scaling more correctly but I'm still hoping we can provide a reasonable UX when those are not provided.

If we investigate more and that turns out to be unfeasible then we should add a warning here. So rdar://99205219 can track that eventuality as well if need be.

CC: @mportiz08

@ethan-kusters
Copy link
Contributor Author

@swift-ci please test

@ethan-kusters ethan-kusters force-pushed the add-page-image-directive branch from 2382375 to 4480239 Compare September 15, 2022 18:13
@ethan-kusters
Copy link
Contributor Author

@swift-ci please test

@ethan-kusters ethan-kusters marked this pull request as ready for review September 15, 2022 18:13
@ethan-kusters ethan-kusters force-pushed the add-page-image-directive branch from 4480239 to 6d702ee Compare September 15, 2022 18:16
@ethan-kusters
Copy link
Contributor Author

@swift-ci please test

/// A basic XML parser that extracts the first `id` attribute found in the given SVG.
///
/// This is a single-purpose tool and should not be used for general-purpose SVG parsing.
enum SVGIDExtractor {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we still need a warning when there's no ID?

@@ -172,18 +177,32 @@ public struct DataAsset: Codable {
/// depending on the system's appearance.
public var variants = [DataTraitCollection: URL]()

/// The metadata associated with each variant.
public var metadata = [URL : Metadata]()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A data modeling question: What's the benefit of having metadata per variant instead of per asset? Would different variants of an SVG ever have different IDs?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's exactly the benefit: The ID is internal to the SVG and I think generally unknown to the developer/author so we can't depend on having the same ID per variant. I can imagine that in most cases the ID will be the same across variants but we can't depend on that without the potential of running into confusing breakage down the road.

Future metadata (maybe image size for a PNG?) - would also be variant (file) specific, not asset specific.

@@ -210,6 +229,21 @@ public struct DataAsset: Codable {

}

extension DataAsset {
/// Metadata specific to this data asset.
public struct Metadata: Codable, Equatable {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm trying to imagine how we would grow/scale/expand this. Do we know of any other metadata that we plan on adding in the future? Is there going to be groups of applicable metadata per file type?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know of any at the moment. Maybe image size would be a reasonable addition in the future? Or maybe an image could be associated with a device frame via metadata?

In the actual JSON model I tried to make this as lightweight as possible (just an additional property in the variant), but here it seemed simpler and more ergonomic to introduce a wrapper struct.

/// A basic XML parser that extracts the first `id` attribute found in the given SVG.
///
/// This is a single-purpose tool and should not be used for general-purpose SVG parsing.
enum SVGIDExtractor {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, what will happen if a custom SVG icon doesn't have an ID?

@ethan-kusters
Copy link
Contributor Author

@swift-ci please test macOS platform

@ethan-kusters
Copy link
Contributor Author

@swift-ci please test macOS platform

@ethan-kusters
Copy link
Contributor Author

@swift-ci please test

@ethan-kusters ethan-kusters force-pushed the add-page-image-directive branch from 6d702ee to b9260f2 Compare September 16, 2022 08:51
@ethan-kusters
Copy link
Contributor Author

@swift-ci please test

@ethan-kusters ethan-kusters force-pushed the add-page-image-directive branch from b9260f2 to 3c528c9 Compare September 16, 2022 08:59
@ethan-kusters
Copy link
Contributor Author

@swift-ci please test

@ethan-kusters ethan-kusters force-pushed the add-page-image-directive branch from 3c528c9 to f0140c4 Compare September 16, 2022 09:11
@ethan-kusters
Copy link
Contributor Author

@swift-ci please test

@ethan-kusters ethan-kusters force-pushed the add-page-image-directive branch from f0140c4 to f7a515c Compare September 16, 2022 09:18
@ethan-kusters
Copy link
Contributor Author

@swift-ci please test

Adds a new `@PageImage` directive that can be placed inside a page’s
`@Metadata` directive block to allow for customizing the images that
will be used to represent the page in the navigator, hero, and elsewhere
in the UI.

For article-only catalogs it’s likely helpful to distinguish between
articles with custom icons- in the same way Swift-DocC has different
icons for different kinds of symbol pages.

`@PageImage` accepts the following parameters:

- purpose: The purpose of the image. One of the following:
    - icon: This image should be used in place of wherever a generic
            icon for the page would usually be rendered.

    - card: This image should be used whenever rendering a card
            representing this page.

- source: A string containing the name of an image in your DocC catalog.

- alt: (optional) A string containing a description of the image.

Example:

   # Feeding a Sloth

   @metadata {
     @PageImage(purpose: icon, source: "leaf.svg", alt: "An icon representing a leaf.")
   }

`@PageImage` is described on the Swift forums here:
https://forums.swift.org/t/supporting-more-types-of-documentation-with-swift-docc/59725#pageimage-22

Dependencies:

- swiftlang/swift-docc-render#417

Resolves rdar://97739580
@ethan-kusters ethan-kusters force-pushed the add-page-image-directive branch from f7a515c to 0693f90 Compare September 16, 2022 10:29
@ethan-kusters
Copy link
Contributor Author

@swift-ci please test

@ethan-kusters ethan-kusters merged commit 0a38abd into swiftlang:main Sep 16, 2022
@ethan-kusters ethan-kusters deleted the add-page-image-directive branch September 16, 2022 10:39
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.

3 participants