-
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 all 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 |
---|---|---|
|
@@ -223,4 +223,97 @@ 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) | ||
} | ||
|
||
func testCopyToClipboardNoFeatureFlag() async throws { | ||
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, false) | ||
} | ||
|
||
func testNoCopyToClipboardNoFeatureFlag() async throws { | ||
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.syntax, "swift, nocopy") | ||
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?