-
Notifications
You must be signed in to change notification settings - Fork 147
Add copy-to-clipboard support to code blocks #1273
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
base: main
Are you sure you want to change the base?
Changes from 9 commits
53ee456
753c0dc
12b2d50
39d2ad3
8ce67d3
08bd353
bb50fb9
1f4894d
3ae43b1
2cf5ac1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -47,7 +47,41 @@ struct RenderContentCompiler: MarkupVisitor { | |
|
||
mutating func visitCodeBlock(_ codeBlock: CodeBlock) -> [any RenderContent] { | ||
// Default to the bundle's code listing syntax if one is not explicitly declared in the code block. | ||
return [RenderBlockContent.codeListing(.init(syntax: codeBlock.language ?? bundle.info.defaultCodeListingLanguage, code: codeBlock.code.splitByNewlines, metadata: nil))] | ||
|
||
if FeatureFlags.current.isExperimentalCodeBlockEnabled { | ||
|
||
struct ParsedOptions { | ||
var lang: String? | ||
var nocopy = false | ||
} | ||
|
||
func parseLanguageString(_ input: String?) -> ParsedOptions { | ||
guard let input else { return ParsedOptions() } | ||
|
||
let parts = input | ||
.split(separator: ",") | ||
.map { $0.trimmingCharacters(in: .whitespaces) } | ||
|
||
var options = ParsedOptions() | ||
|
||
for part in parts { | ||
let lower = part.lowercased() | ||
if lower == "nocopy" { | ||
options.nocopy = true | ||
} else if options.lang == nil { | ||
options.lang = part | ||
} | ||
} | ||
return options | ||
} | ||
|
||
let options = parseLanguageString(codeBlock.language) | ||
Comment on lines
+53
to
+78
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I understand correctly, I think that we'd want to parse the code block's languages string into its components even when the feature flags is not enabled. Otherwise, a string like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I feel like i have the opposite opinion here. If someone has written There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A way we can soften the blow of working with this is to add this feature flag to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's fair. I can imagine different people being surprised about both behaviors. @DebugSteven has already added this flag to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As a mental note for myself and @QuietMisdreavus: |
||
|
||
return [RenderBlockContent.codeListing(.init(syntax: options.lang ?? bundle.info.defaultCodeListingLanguage, code: codeBlock.code.splitByNewlines, metadata: nil, copyToClipboard: !options.nocopy))] | ||
|
||
} else { | ||
return [RenderBlockContent.codeListing(.init(syntax: codeBlock.language ?? bundle.info.defaultCodeListingLanguage, code: codeBlock.code.splitByNewlines, metadata: nil, copyToClipboard: false))] | ||
} | ||
} | ||
|
||
mutating func visitHeading(_ heading: Heading) -> [any RenderContent] { | ||
|
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -143,6 +143,27 @@ add a new line and terminate the code listing by adding another three backticks: | |||||||||
instead of tabs so that DocC preserves the indentation when compiling your | ||||||||||
documentation. | ||||||||||
|
||||||||||
#### Formatting Code Listings | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. minor: I'm not sure how I feel about this heading. With the current content it feels like this section is more about enabling or configuring additional features for code listings. Two different headings that could fit this could be:
Suggested change
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That said; we don't require user-facing documentation for experimental features. Instead, we normally document them when we mark them as stable / non-experimental. It could be easier to document the new code block features when there's more of them. Otherwise there's some risk of churn from rewriting and restructuring the content as pieces of the same experimental feature are added. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you for all of this feedback. I’ve gone ahead and removed the documentation for this feature from this PR. You raised a good point with there being a lot of churn while this feature remains experimental. I’ve taken all of your suggestions for the documentation and we’ll add the final docs if/when these features stablize. |
||||||||||
|
||||||||||
A copy-to-clipboard button is added to code listings by default behind the | ||||||||||
feature flag `--enable-experimental-code-block`. | ||||||||||
DebugSteven marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||
This renders a copy button in the top-right cotner of the code listing in | ||||||||||
generated documentation. When clicked, it copies the contents of the code | ||||||||||
block to the clipboard. | ||||||||||
DebugSteven marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||
|
||||||||||
If you don't want a code block to have a copy-to-clipboard button, you can | ||||||||||
include the `nocopy` option after the name of the programming language to | ||||||||||
disable it for that code listing: | ||||||||||
DebugSteven marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||
|
||||||||||
```swift, nocopy | ||||||||||
struct Sightseeing: Activity { | ||||||||||
func perform(with sloth: inout Sloth) -> Speed { | ||||||||||
sloth.energyLevel -= 10 | ||||||||||
return .slow | ||||||||||
} | ||||||||||
} | ||||||||||
``` | ||||||||||
DebugSteven marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||
|
||||||||||
DocC uses the programming language you specify to apply the correct syntax | ||||||||||
color formatting. For the example above, DocC generates the following: | ||||||||||
|
||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -223,4 +223,52 @@ class RenderContentCompilerTests: XCTestCase { | |
XCTAssertEqual(documentThematicBreak, thematicBreak) | ||
} | ||
} | ||
|
||
func testCopyToClipboard() async throws { | ||
enableFeatureFlag(\.isExperimentalCodeBlockEnabled) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Question: is there a corresponding test that checks that code listings don't have a copy-to-clipboard button when the feature flag isn't set? This could be as small as adding There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I didn't see tests that already had Code Listings on them, so I opted to add a new test. If there's a place you'd prefer me to add this assertion, let me know and I can do that instead. |
||
|
||
let (bundle, context) = try await testBundleAndContext() | ||
var compiler = RenderContentCompiler(context: context, bundle: bundle, identifier: ResolvedTopicReference(bundleID: bundle.id, path: "/path", fragment: nil, sourceLanguage: .swift)) | ||
|
||
let source = #""" | ||
```swift | ||
let x = 1 | ||
``` | ||
"""# | ||
let document = Document(parsing: source) | ||
|
||
let result = document.children.flatMap { compiler.visit($0) } | ||
|
||
let renderCodeBlock = try XCTUnwrap(result[0] as? RenderBlockContent) | ||
guard case let .codeListing(codeListing) = renderCodeBlock else { | ||
XCTFail("Expected RenderBlockContent.codeListing") | ||
return | ||
} | ||
|
||
XCTAssertEqual(codeListing.copyToClipboard, true) | ||
} | ||
|
||
func testNoCopyToClipboard() async throws { | ||
enableFeatureFlag(\.isExperimentalCodeBlockEnabled) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Question: Do we want to test the behavior when a code block contains There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added another test for this as well. It shows that when the feature flag isn't enabled and a code block contains |
||
|
||
let (bundle, context) = try await testBundleAndContext() | ||
var compiler = RenderContentCompiler(context: context, bundle: bundle, identifier: ResolvedTopicReference(bundleID: bundle.id, path: "/path", fragment: nil, sourceLanguage: .swift)) | ||
|
||
let source = #""" | ||
```swift, nocopy | ||
let x = 1 | ||
``` | ||
"""# | ||
let document = Document(parsing: source) | ||
|
||
let result = document.children.flatMap { compiler.visit($0) } | ||
|
||
let renderCodeBlock = try XCTUnwrap(result[0] as? RenderBlockContent) | ||
guard case let .codeListing(codeListing) = renderCodeBlock else { | ||
XCTFail("Expected RenderBlockContent.codeListing") | ||
return | ||
} | ||
|
||
XCTAssertEqual(codeListing.copyToClipboard, false) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have any plans to raise diagnostics here if we encounter an unknown option?
For example, the code below will parse
swift, nocpoy
aslang: nocpoy
,nocopy: false
and the only way for the developer to notice this is to see that the rendered code listing has a copy-to-clipboard button and doesn't have syntax highlighting.If we want the possibility of raising diagnostics from this (or future) code block options, it probably needs be parsed earlier in the build, before "rendering".
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be a good idea to raise diagnostics for unknown options. I’m not familiar with DocC diagnostics and I don’t understand why it would need to be emitted earlier. Could you give me some more info?