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.

Sign up for GitHub

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

Jump to bottom

Add copy code-block plugin #1043

Merged
merged 10 commits into from
Mar 4, 2020
Merged

Add copy code-block plugin #1043

merged 10 commits into from
Mar 4, 2020

Conversation

yash-chowdhary
Copy link
Contributor

@yash-chowdhary yash-chowdhary commented Feb 15, 2020 •
edited
Loading

What is the purpose of this pull request? (put "X" next to an item, remove the rest)

• [x] New feature

Closes #562

What is the rationale for this request?
Lets users copy code in a given code block.
Since one of MarkBind's major use cases is project documentation, copying code blocks would be very beneficial to users.

What changes did you make? (Give an overview)

Demo

ezgif com-video-to-gif-2

Is there anything you'd like reviewers to focus on?
N/A

Testing instructions:

  • Since this is a plugin, add the following to site.json : 
    ...
"plugins": [
    "codeBlockCopyButtons"
]
...

Proposed commit message: (wrap lines at 72 characters)
Add copy code-block plugin

@yash-chowdhary yash-chowdhary marked this pull request as ready for review February 15, 2020 15:21
@openorclose
Copy link
Contributor

What's the reason to use clipboard.js, a rather large file, over a shorter code snippet?

Is it for browser compatibility?

@yash-chowdhary
Copy link
Contributor Author

In part, yes. I was also facing some difficulty in using document.execCommand("copy") as it requires the id of the element and our <pre> elements don't have ids.

@openorclose
Copy link
Contributor

In part, yes. I was also facing some difficulty in using document.execCommand("copy") as it requires the id of the element and our <pre> elements don't have ids.

The button is located inside the <pre> element, so you should be able to access it with event.target.parentElement.

@yash-chowdhary
Copy link
Contributor Author

yash-chowdhary commented Feb 16, 2020 •
edited
Loading

I've removed the dependency on clipboard.js.
Updated the PR description accordingly.
Ready for review.

marvinchin
marvinchin requested changes Feb 17, 2020
View reviewed changes
Copy link
Contributor

@marvinchin marvinchin left a comment

Choose a reason for hiding this comment

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

Thanks for looking at this - I have a couple of questions 🙂

src/constants.js Outdated Show resolved Hide resolved
docs/userGuide/plugins/copyCode.mbdf Outdated Show resolved Hide resolved
asset/css/markbind.css Outdated Show resolved Hide resolved
asset/css/markbind.css Outdated Show resolved Hide resolved
src/plugins/copyCode.js Outdated Show resolved Hide resolved
src/plugins/copyCode.js Outdated Show resolved Hide resolved
src/plugins/copyCode.js Outdated Show resolved Hide resolved
src/plugins/copyCode.js Outdated Show resolved Hide resolved
src/plugins/copyCode.js Outdated Show resolved Hide resolved
src/plugins/copyCode.js Outdated Show resolved Hide resolved
marvinchin
marvinchin reviewed Feb 21, 2020
View reviewed changes
Copy link
Contributor

@marvinchin marvinchin left a comment

Choose a reason for hiding this comment

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

Just one last question :)

asset/css/markbind.css Outdated Show resolved Hide resolved
@yash-chowdhary
Copy link
Contributor Author

yash-chowdhary commented Feb 25, 2020 •
edited
Loading

Rebased on master. Ready for review

marvinchin
marvinchin approved these changes Feb 25, 2020
View reviewed changes
Copy link
Contributor

@marvinchin marvinchin left a comment

Choose a reason for hiding this comment

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

LGTM :)

Just a follow-up thought: It feels kinda odd that we add plugin styles to the main markbind.css file. Perhaps we should come up with a better way for plugin writers to keep assets contained within the plugin folder itself.

marvinchin
marvinchin reviewed Feb 26, 2020
View reviewed changes
Copy link
Contributor

@marvinchin marvinchin left a comment

Choose a reason for hiding this comment

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

@yash-chowdhary seems like the version change wasn't caused by changes made this PR, so let's fix it in another one. I believe #1055 should fix it - let's drop that change here and rebase it on top of that instead.

@marvinchin marvinchin added this to the v2.11.1 milestone Feb 27, 2020
@yash-chowdhary
Copy link
Contributor Author

Rebased on master. Resolved conflicts.

@marvinchin marvinchin merged commit b8413c6 into MarkBind:master Mar 4, 2020
@yamgent yamgent added the pr.NewFeature 🆕 Enable users (authors/readers) to do something new label Mar 7, 2020
Tejas2805 added a commit to Tejas2805/markbind that referenced this pull request Mar 7, 2020
@Tejas2805
Merge branch 'master' of https://github.com/MarkBind/markbind into co…
a5c6f32 
…nvert-to-code-block

* 'master' of https://github.com/MarkBind/markbind:
  Allow changing parameter properties (MarkBind#1075)
  Custom timezone for built-in timestamp (MarkBind#1073)
  Fix reload inconsistency when updating frontmatter (MarkBind#1068)
  Implement an api to ignore content in certain tags (MarkBind#1047)
  Enable AppVeyor CI (MarkBind#1040)
  Add heading and line highlighting to code blocks (MarkBind#1034)
  Add dividers and fix bug in siteNav (MarkBind#1063)
  Fixed navbar no longer covers modals (MarkBind#1070)
  Add copy code-block plugin (MarkBind#1043)
  Render plugins on dynamic resources (MarkBind#1051)
  Documentation for Implement no-* attributes for <box>  (MarkBind#1042)
  Migrate to bootstrap-vue popovers (MarkBind#1033)
  Refactor preprocess and url processing functions (MarkBind#1026)
  Add pageNav to Using Plugins Page (MarkBind#1062)

# Conflicts:
#	docs/userGuide/syntax/siteNavigationMenus.mbdf
Tejas2805 added a commit to Tejas2805/markbind that referenced this pull request Mar 9, 2020
@Tejas2805
Merge branch 'master' of https://github.com/MarkBind/markbind
31d0325 
* 'master' of https://github.com/MarkBind/markbind:
  2.12.0
  Update outdated test files
  Update vue-strap version to v2.0.1-markbind.37
  Fix refactor to processDynamicResources (MarkBind#1092)
  Implement lazy page building for markbind serve (MarkBind#1038)
  Add warnings for conflicting/deprecated component attribs (MarkBind#1057)
  Allow changing parameter properties (MarkBind#1075)
  Custom timezone for built-in timestamp (MarkBind#1073)
  Fix reload inconsistency when updating frontmatter (MarkBind#1068)
  Implement an api to ignore content in certain tags (MarkBind#1047)
  Enable AppVeyor CI (MarkBind#1040)
  Add heading and line highlighting to code blocks (MarkBind#1034)
  Add dividers and fix bug in siteNav (MarkBind#1063)
  Fixed navbar no longer covers modals (MarkBind#1070)
  Add copy code-block plugin (MarkBind#1043)
  Render plugins on dynamic resources (MarkBind#1051)
  Documentation for Implement no-* attributes for <box>  (MarkBind#1042)
  Migrate to bootstrap-vue popovers (MarkBind#1033)
  Refactor preprocess and url processing functions (MarkBind#1026)
  Add pageNav to Using Plugins Page (MarkBind#1062)
marvinchin pushed a commit that referenced this pull request Apr 10, 2020
@yash-chowdhary
Add copy code-block plugin (#1043)
ddaa948
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@openorclose openorclose openorclose left review comments

@marvinchin marvinchin marvinchin approved these changes

@yamgent yamgent Awaiting requested review from yamgent

Assignees
No one assigned
Labels
pr.NewFeature 🆕 Enable users (authors/readers) to do something new
Projects
None yet
Milestone
v2.12.0
Development

Successfully merging this pull request may close these issues.

Give a copy button in code blocks
4 participants
@yash-chowdhary @openorclose @marvinchin @yamgent