-
Notifications
You must be signed in to change notification settings - Fork 906
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
[BSv5] Refactor Click-to-copy #1447
Conversation
c67788e
to
9f7511b
Compare
9f7511b
to
aa7bb07
Compare
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.
Works nicely. See inline comments, thanks.
assets/scss/_code.scss
Outdated
@@ -1,101 +1,84 @@ | |||
// Code formatting. | |||
|
|||
.td-content { | |||
// Highlighted code. |
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.
It's hard to follow what has changed in this file. Could you consider not reformatting it in this PR so that the diffs are more clearly seen?
Did you only change the lines for the c2c button?
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.
There's nothing new in this file. I deleted all the styles for the tooltip and prettified it.
assets/scss/_code.scss
Outdated
transition: visibility 0s, opacity 0.5s linear; | ||
} | ||
// click-to-copy button | ||
button { |
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'd prefer that you use a class name specific to this feature (and add the class via the js):
button { | |
button.td-click-to-copy { |
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.
Updated here and added the class in the js file
@@ -148,6 +148,24 @@ guessSyntax: true | |||
{{< /tab >}} | |||
{{< /tabpane >}} | |||
|
|||
If you are using a Docsy version later than `0.6.0`, the code blocks show a |
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 you are using a Docsy version later than `0.6.0`, the code blocks show a | |
If you are using a Docsy version `0.6.0` or later, code blocks show a |
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.
This does not work for 0.6.0
and we haven't definitely decided on the next version number. I think "later than" covers that. About the article "the", although I know I don't really need it here, I try to never omit it based on this guideline: https://developers.google.com/style/articles
I've merged #1454. Apologies for the resulting merge conflicts, but as I mention in the PR, I should have done this earlier. |
@geriom - a more general question, why not use clipboardjs.com? |
aa7bb07
to
3692428
Compare
I don't think we need an additional dependency for a straightforward feature. I can see how that library would be very helpful for single-page applications with lots of dynamic content, but here I don't see a big benefit. |
Refactor of PR google#1245. Bootstrap5-compatible code.
3692428
to
134127d
Compare
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 the updates!
Refactor of PR #1245. Bootstrap5-compatible code.
Closes #1267