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

Add copy code button to code fragments in descriptions #4488

Merged
merged 11 commits into from
Mar 21, 2023

Conversation

jorg-vr
Copy link
Contributor

@jorg-vr jorg-vr commented Mar 16, 2023

This pull request adds a copy code button to code fragments in descriptions.

I matched on the <pre><code></code></pre> html tags in descriptions to match all logical cases. This tag is also correctly used by markdown.

image

Should the copying fail, it selects the text and suggests to use Ctrl + c

Closes #3532

@jorg-vr jorg-vr added the enhancement A change that isn't substantial enough to be called a feature label Mar 16, 2023
@jorg-vr jorg-vr self-assigned this Mar 16, 2023
@jorg-vr jorg-vr marked this pull request as ready for review March 16, 2023 15:40
@jorg-vr jorg-vr requested a review from a team as a code owner March 16, 2023 15:40
@jorg-vr jorg-vr requested review from bmesuere and niknetniko and removed request for a team March 16, 2023 15:40
Copy link
Member

@niknetniko niknetniko left a comment

Choose a reason for hiding this comment

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

  • For some reason it doesn't seem to work for me in Chrome (using version 111).
  • The error message that appears is also not that useful I think ("Druk Ctrl-C om te kopiëren")
  • Some (or most?) of the HTML problem statements don't use a <code> tag, only a <pre>, so they don't get a button.

@jorg-vr jorg-vr marked this pull request as draft March 17, 2023 12:23
@jorg-vr
Copy link
Contributor Author

jorg-vr commented Mar 17, 2023

  • For some reason it doesn't seem to work for me in Chrome (using version 111).

Fixed, it needed extra permissions because it is in an Iframe

  • The error message that appears is also not that useful I think ("Druk Ctrl-C om te kopiëren")

I now actually select the text if copying failed.

  • Some (or most?) of the HTML problem statements don't use a <code> tag, only a <pre>, so they don't get a button.

These problem statements do not follow the specification: https://docs.dodona.be/nl/references/exercise-description/#blokken-code Thus we cannot guarantee the layout.

I did update the matching form code to pre code, to avoid matching inline code

@jorg-vr jorg-vr marked this pull request as ready for review March 17, 2023 13:43
@jorg-vr jorg-vr requested a review from niknetniko March 17, 2023 13:43
Copy link
Member

@niknetniko niknetniko left a comment

Choose a reason for hiding this comment

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

Would it be an idea to trim the copied code? For example, when copying this code:
image
It will result in this being copied:
image

Looks good otherwise.

@bmesuere bmesuere added the deploy naos Request a deployment on naos label Mar 19, 2023
@bmesuere bmesuere temporarily deployed to naos March 19, 2023 14:47 — with GitHub Actions Inactive
@github-actions github-actions bot removed the deploy naos Request a deployment on naos label Mar 19, 2023
Copy link
Member

@bmesuere bmesuere left a comment

Choose a reason for hiding this comment

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

Something goes wrong with inline code. They are now rendered as a block:
Before:
image

After:
image

The background of the "button" is transparent which causes un unwanted effect:
image

@jorg-vr
Copy link
Contributor Author

jorg-vr commented Mar 20, 2023

Something goes wrong with inline code. They are now rendered as a block:

Forgot to push the fix for this

Would it be an idea to trim the copied code?

That was an artifact of adding the copy button inside the codeblock. The copy button is now no longer included in the copied text. I prefer this solution to a trim, because leading and ending whitespace might be relevant in some code examples.

The background of the "button" is transparent which causes un unwanted effect:

Fixed

@bmesuere bmesuere added the deploy naos Request a deployment on naos label Mar 21, 2023
@bmesuere bmesuere temporarily deployed to naos March 21, 2023 09:17 — with GitHub Actions Inactive
@github-actions github-actions bot removed the deploy naos Request a deployment on naos label Mar 21, 2023
Copy link
Member

@bmesuere bmesuere left a comment

Choose a reason for hiding this comment

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

Can we also replace the existing copy buttons by the new web component?

@jorg-vr jorg-vr merged commit 39fd61f into develop Mar 21, 2023
@jorg-vr jorg-vr deleted the enhance/copy-code-fragments branch March 21, 2023 10:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A change that isn't substantial enough to be called a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Copy code fragment to clipboard
3 participants