Skip to content

Conversation

ethan-kusters
Copy link
Contributor

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

Summary

Updates the logic for @CallToAction buttons that are attached to sampleCode pages and use the link purpose to render with a more specific “View Source” title instead of the more generic “Visit” title.

This makes the intent of the button more clear. Regular article pages should continue to have the same behavior.

Testing

Create an article with @CallToAction directive using that provides a url and link purpose and a @PageKind(sampleCode) directive. Confirm that the call to action button is labeled "View Source".

@Metadata {
    @PageKind(sampleCode)
    @CallToAction(url: "https://github.com/apple/swift-docc", purpose: link)
}

Screenshot 2023-04-25 at 4 06 25 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

Updates the logic for `@CallToAction` buttons that are attached
to `sampleCode` pages and use the `link` purpose to render with
a more specific “View Source” title instead of the more generic
“Visit” title.

This makes the intent of the button more clear.

Resolves rdar://108013602
@ethan-kusters
Copy link
Contributor Author

@swift-ci please test

Copy link
Contributor

@d-ronnqvist d-ronnqvist left a comment

Choose a reason for hiding this comment

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

This looks good to me.

Just two non-blocking suggestions for the implementations of the deprecated properties.

/// The label that will be applied to a Call to Action with this purpose if it doesn't provide
/// a separate label.
@available(*, deprecated, message: "Replaced with 'CallToAction.buttonLabel(for:)'.")
public var defaultLabel: String {
Copy link
Contributor

Choose a reason for hiding this comment

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

should this internally call defaultLabel(for: nil)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea: c83c4a3.

/// indirectly via ``purpose``.
@available(*, deprecated, renamed: "buttonLabel(for:)")
public var buttonLabel: String {
if let label = label {
Copy link
Contributor

Choose a reason for hiding this comment

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

should this internally call buttonLabel(for: nil)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea: c83c4a3.

@ethan-kusters
Copy link
Contributor Author

@swift-ci please test

Copy link
Contributor

@QuietMisdreavus QuietMisdreavus left a comment

Choose a reason for hiding this comment

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

Looks good!

@ethan-kusters ethan-kusters merged commit 31ba814 into swiftlang:main Apr 26, 2023
@ethan-kusters ethan-kusters deleted the sample-code-link-title branch April 26, 2023 18:39
ethan-kusters added a commit to ethan-kusters/swift-docc that referenced this pull request Apr 26, 2023
…g#566)

Updates the logic for `@CallToAction` buttons that are attached
to `sampleCode` pages and use the `link` purpose to render with
a more specific “View Source” title instead of the more generic
“Visit” title.

This makes the intent of the button more clear.

Resolves rdar://108013602
ethan-kusters added a commit that referenced this pull request Apr 26, 2023
)

Updates the logic for `@CallToAction` buttons that are attached
to `sampleCode` pages and use the `link` purpose to render with
a more specific “View Source” title instead of the more generic
“Visit” title.

This makes the intent of the button more clear.

Resolves rdar://108013602
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