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

@Image seems to be ignored in Articles, one has to use ![]() #274

Closed
ktoso opened this issue Jun 2, 2022 · 16 comments · Fixed by #381
Closed

@Image seems to be ignored in Articles, one has to use ![]() #274

ktoso opened this issue Jun 2, 2022 · 16 comments · Fixed by #381
Assignees
Labels
bug Something isn't working

Comments

@ktoso
Copy link
Member

ktoso commented Jun 2, 2022

While reading abut docc one learns about directives and @Image so one wants to use it... yet it does not seem to work in Articles, and one has to resort to the markdown method ![]() this is rather confusing, as no warning is issued about the "wrong" use or anything else, leading one to dig through and start guessing if one messed up paths, names or something else.

Expected behavior

Option 1: Can we make @Image(source: work in articles, in addition to the markdown style?

Option 2: Actually banning @Image could be an option as in Articles it might be better to use ![]()? But then we should issue warnings when this not supported use is detected.

Actual behavior

When using @Image(source: "example.png", alt: "...") nothing happens, not even an empty <img> is rendered.

Steps to Reproduce

Make some article, and use @image in it; I did so in the following PR and it did not work: apple/swift-distributed-actors@0262b75 (open source package)

rdar://94284119

@ktoso ktoso added the bug Something isn't working label Jun 2, 2022
@Kyle-Ye
Copy link
Collaborator

Kyle-Ye commented Jun 5, 2022

@image and alike are Block Directive elements only avaiable for Swift-DocC's tutorial files.

I do not think we should support it in normal markdown file.

Maybe the best we can do is add a warning to indict this for possible misuse.

But if we add this warning and someone really wants the "@image (source:" in text form, should we add the ability to suppress this warning(via a flag)?

cc @franklinsch

@Kyle-Ye Kyle-Ye self-assigned this Jun 5, 2022
@ktoso
Copy link
Member Author

ktoso commented Jun 5, 2022

Issuing a warning sounds good to me -- as long as we do something to inform that "this indeed is intended to not work" with a warning 👍

@franklinsch
Copy link
Member

A warning and a fix-it to help you convert to the plain Markdown syntax would be great indeed! cc @binamaniar

@franklinsch
Copy link
Member

@ethan-kusters
Copy link
Contributor

Should we consider just supporting @Image in this context? I'd be interested in the argument against this.

Markdown's image syntax is one of the things I see newcomers to Markdown get tripped up on the most and @Image is fairly easily understandable. Additionally- it seems overly confusing that Swift-DocC allows @Image directive use in some contexts but not others.

Of course, Swift-DocC should continue to support the plain Markdown syntax, but if a user reaches for the @Image syntax I don't personally see a reason why it shouldn't work.

@franklinsch
Copy link
Member

franklinsch commented Jun 8, 2022

I believe the initial motivation for @Image rather than ![]() in tutorials was to make it more extensible with future directive arguments (![]() only supports specifying the resource URI and alt text, but that's the same for @Image currently). I'm not opposed to supporting @Image in both reference documentation and tutorials, and we'd also support ![]() in tutorials for consistency. These docs will need to be updated: https://www.swift.org/documentation/docc/image

@Kyle-Ye
Copy link
Collaborator

Kyle-Ye commented Jun 8, 2022

Should we consider just supporting @Image in this context? I'd be interested in the argument against this.

Markdown's image syntax is one of the things I see newcomers to Markdown get tripped up on the most and @Image is fairly easily understandable. Additionally- it seems overly confusing that Swift-DocC allows @Image directive use in some contexts but not others.

Of course, Swift-DocC should continue to support the plain Markdown syntax, but if a user reaches for the @Image syntax I don't personally see a reason why it shouldn't work.

-1 for this. Personally, I do not think @Image and ![]() are interchangeable.

Since @Image will look for the best suitable varient of the image (like xx@dark.png) while ![]() specify the determined url of the image.

eg.
In a dark and 3x context. There is a.png and a@dark.png

@Image(source: a.png, alt: a) -> a@dark.png
![a](a.png) -> a.png

We can make ![a](a.png) to look for varient too. But I think we should respect the common Markdown rule on the Internet and also perserve the ability to create a determined url image.

@franklinsch
Copy link
Member

DocC emits variant information (dark, 2x, etc.) for images in ![](), too (see https://www.swift.org/documentation/docc/formatting-your-documentation-content#Add-Images-to-Your-Content). I'd need to check to make sure, but I do believe ![]() and @Image are equivalent, apart from where you're allowed to use them in DocC content.

@ethan-kusters
Copy link
Contributor

ethan-kusters commented Jun 8, 2022

We can make ![a](a.png) to look for varient too. But I think we should respect the common Markdown rule on the Internet and also perserve the ability to create a determined url image.

Right as @franklinsch said- the ![a](a) syntax should look for variants so if there's situations where it's not we should fix it. Additionally- the ![a](a) syntax in DocC has never really behaved the way it does normally in Markdown where you provide a relative or absolute path. It's always been just the name of the image, the same way @Image works.

@Kyle-Ye
Copy link
Collaborator

Kyle-Ye commented Jun 9, 2022

We can make ![a](a.png) to look for varient too. But I think we should respect the common Markdown rule on the Internet and also perserve the ability to create a determined url image.

Right as @franklinsch said- the ![a](a) syntax should look for variants so if there's situations where it's not we should fix it. Additionally- the ![a](a) syntax in DocC has never really behaved the way it does normally in Markdown where you provide a relative or absolute path. It's always been just the name of the image, the same way @Image works.

Got it.

One more question here. How we should treat the external link for ![]()? Such as ![a](https://example.com/a.png). Currently we just treat it as a 1x and light trait. Should there be a way to look for variants like the local assets?

@franklinsch
Copy link
Member

franklinsch commented Jun 9, 2022

Hmm, I don't think there would be a way for DocC to find what variants are available automatically. However, if you could manually specify the available variants of images, I think that'd be nice. This would require more arguments so that @Image syntax might be more appropriate:

@Image(source: "https://example.com/a.png", alt: "Alt text") {
  @Variant(trait: "dark@2x", source: "https://example.com/a-dark.png")
}

@Kyle-Ye
Copy link
Collaborator

Kyle-Ye commented Jun 9, 2022

Hmm, I don't think there would be a way for DocC to find what variants are available automatically. However, if you could manually specify the available variants of images, I think that'd be nice. This would require more arguments so that @Image syntax might be more appropriate:

@Image(source: "https://example.com/a.png", alt: "Alt text") {

  @Variant(trait: "dark@2x", source: "https://example.com/a-dark.png")

}

Let's summary on what we agree on

  1. Split Add TutorialDirectiveMisuseChecker #278 to a. Directive related improvement and b. markdown related fix.(We'd better merge apple/markdown#50 and b first then a otherwise a would have to use a buggy workaround)
  2. Support @Image instead of give a warning and solution suggestion. (This PR need some major update to reflect this new change. I'll make the PR draft and reenable it for review later)
  3. Future direction. When solving [SR-16026] Add external media support for DocC tutorial #172, we may consider adding @Varients support to @Image so that we can reference variants for external image too.

Any suggestion or supplement on the above summary is welcomed.

@ktoso
Copy link
Member Author

ktoso commented Jul 25, 2022

Cool, looking forward to the @Image support then; I agree that's the most reasonable.

@Kyle-Ye
Copy link
Collaborator

Kyle-Ye commented Oct 5, 2022

Mark this is closed since #381 is merged and should fix this issue.

@Kyle-Ye Kyle-Ye closed this as completed Oct 5, 2022
@ktoso
Copy link
Member Author

ktoso commented Oct 5, 2022

Cool, looking forward to trying that out 👍

@Kyle-Ye
Copy link
Collaborator

Kyle-Ye commented Oct 5, 2022

Cool, looking forward to trying that out 👍

You can try it with swift-DEVELOPMENT-SNAPSHOT-2022-10-03-a. Or just clone the latest swift-docc and swift-docc-render(This is needed for Image/Video caption support).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
4 participants