-
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?
Conversation
@swift-ci please test |
Since this is adding new user-facing syntax I'm adding the |
There is a forum thread for this here. It would be good for that thread to cover some discussion on future directions so that we feel comfortable that this syntax can scale to accommodate those future directions (or intentionally not support them if we believe that each would benefit from a different user-facing syntax). |
c89047e
to
d0b75d8
Compare
d0b75d8
to
1f4894d
Compare
…rd and other code block annotations
@swift-ci please test |
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.
Thanks for opening this PR.
The code looks good to me with some questions about how nocopy
option should work when the feature flag isn't enabled. If the current behavior is intended then that code doesn't need to change.
I have some non-blocking feedback on the developer-facing documentation for this. Because the feature flag help-text mentions additional features that aren't added in this PR I suspect that this documentation (and possibly also the syntax) could frequently as more features are added. If you are fine with updating (and possibly rewriting parts of) the documentation as new experimental features as added, then you can keep it in the pull request. An alternative would be to write the documentation when removing the experimental status from these features, when the scope and syntax has settled.
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) |
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.
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 swift, nocopy
wouldn't produce the any syntax highlighting unless the developer also passes the feature flag.
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 feel like i have the opposite opinion here. If someone has written swift,nocopy
, then they are explicitly aware of this feature flag and should be required to enable it to maintain the expected behavior. Otherwise they're writing a "weird language flag" that we would otherwise know nothing about.
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.
A way we can soften the blow of working with this is to add this feature flag to BundleFeatureFlags
to allow it to be set in Info.plist, like i did with the overloaded-symbol presentation in #891. That way, authors can set the feature flag in the same commit that they start to use the nocopy
flags, and it should Just Work™.
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.
That's fair. I can imagine different people being surprised about both behaviors.
@DebugSteven has already added this flag to BundleFeatureFlags
, so that seems like a good way for people to adopt this while this feature is experimental.
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.
As a mental note for myself and @QuietMisdreavus:
Looking at the BundleFeatureFlags
code now, we will at some point need to figure out a strategy for deprecating and migrating an experimental flag in the Info.plist to its non-experimental replacement. We haven't needed to do so yet, but it's kind of weird that the container key includes the word "experimental" if it can contain non-experimental flags.
Sources/docc/DocCDocumentation.docc/formatting-your-documentation-content.md
Outdated
Show resolved
Hide resolved
Sources/docc/DocCDocumentation.docc/formatting-your-documentation-content.md
Outdated
Show resolved
Hide resolved
Sources/docc/DocCDocumentation.docc/formatting-your-documentation-content.md
Outdated
Show resolved
Hide resolved
@@ -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 comment
The 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 XCTAssertEqual(codeListing.copyToClipboard, false)
to some existing test.
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 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.
} | ||
|
||
func testNoCopyToClipboard() async throws { | ||
enableFeatureFlag(\.isExperimentalCodeBlockEnabled) |
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.
Question: Do we want to test the behavior when a code block contains swift, nocopy
but the feature flag isn't enabled?
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 added another test for this as well. It shows that when the feature flag isn't enabled and a code block contains swift, nocopy
that codeListing.syntax
will equal the contents of the entire line, swift, nocopy
in this case. Let me know if there is more I should do on that test.
Sources/docc/DocCDocumentation.docc/formatting-your-documentation-content.md
Outdated
Show resolved
Hide resolved
Sources/SwiftDocCUtilities/ArgumentParsing/Subcommands/Convert.swift
Outdated
Show resolved
Hide resolved
var nocopy = false | ||
} | ||
|
||
func parseLanguageString(_ input: String?) -> ParsedOptions { |
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
as lang: 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".
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?
Summary
This PR adds support for a new copy-to-clipboard button in code blocks. When this feature is enabled through the
enable-experimental-code-block
flag, a copy-to-clipboard button is rendered in the top-right corner of all code blocks, allowing users to easily copy its contents. When an author does not want the contents of a code block to be copyable, anocopy
option can be used to disable the copy-to-clipboard button.User Experience
When the
enable-experimental-code-block
flag is used, a copy button will appear in the top-right corner of all code blocks. Clicking the button copies the full contents of the code block to the clipboard and displays a checkmark to confirm success. If a code block includes thenocopy
keyword in the language line, the copy button will not appear on that code block.Implementation Overview
swift-docc
, this change adds a feature flagenable-experimental-code-block
, which enables a copy-to-clipboard button on code blocks by default.nocopy
option from the language line in triple-backtick code blocks to disable the copy button on that code block.copyToClipboard
property was added toRenderBlockContent.codeListing
passed to the renderer.swift-docc-render
. The accompanying branch/PR is here: Add copy-to-clipboard support to code blocks swift-docc-render#961Dependencies
This PR depends on associated changes in
swift-docc-render
to actually render and handle the copy button.Testing
Setup
swift-docc
andswift-docc-render
with the copy-to-clipboard changes.swift-docc
with the feature flagenable-experimental-code-block
and serve it using a local build ofswift-docc-render
.How to Test
enable-experimental-code-block
flag, a copy button will appear in the top-right corner.To disable the copy icon with the feature flag enabled:
```
or by adding a code listing, add thenocopy
option like this:
or like this:
2. Verify the copy button does not appear on this code block.
Checklist
Make sure you check off the following items. If they cannot be completed, provide a reason.
testCopyToClipboard()
inRenderContentCompilerTests.swift
./bin/test
script and it succeededcopyToClipboard
as a property ofCodeListing
inRenderNode.spec.json
. I also added a subsection to Formatting Your Documentation Content. Please let me know if there’s any other documentation I should update.