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 support #330

Merged
merged 5 commits into from
Mar 17, 2023
Merged

Add copy support #330

merged 5 commits into from
Mar 17, 2023

Conversation

XhmikosR
Copy link
Member

@XhmikosR XhmikosR commented Nov 27, 2022

TODO:

  • See if we can get rid of the newlines when copying, otherwise it's annoying/disrupting to add the functionality

@julien-deramond
Copy link
Member

Good idea to add copy support in the blog as well! Feel free to ping me or to assign me the PR if you need a review or something.

@XhmikosR
Copy link
Member Author

I ended up using trimEnd() which is fine in most cases. I think this is a good first implementation.

At some point, we could just drop the clipboard package, but I'll leave that for someone else to implement. And then we need to backport it in blog and icons.

@XhmikosR XhmikosR marked this pull request as ready for review November 30, 2022 07:21
}, 2000)
})

/*clipboard.on('error', () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

@julien-deramond This is commented out. I didn't have the time so I left it here. Feel free to push any suggestions :)

Copy link
Member

Choose a reason for hiding this comment

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

I suppose that we don't want here to embed Popper + use tooltips as we do in Bootstrap doc.

So I'd try something like that (not really tested in depth) if I've understood correctly what you'd like to achieve:

diff --git a/src/assets/js/application.js b/src/assets/js/application.js
index f3c93167..48c33cc5 100644
--- a/src/assets/js/application.js
+++ b/src/assets/js/application.js
@@ -13,9 +13,14 @@ const btnHtml = [
   '</button>',
 '</div>'].join('');
 
+const copyErrorCallout = [
+  '<div id="copy-error-callout" class="d-none alert alert-danger" role="alert">',
+  '</div>'].join('');
+
 document.querySelectorAll('div.highlight')
   .forEach((element) => {
     element.insertAdjacentHTML('beforebegin', btnHtml)
+    element.insertAdjacentHTML('beforeend', copyErrorCallout)
   })
 
 const clipboard = new ClipboardJS('.btn-clipboard', {
@@ -24,6 +29,11 @@ const clipboard = new ClipboardJS('.btn-clipboard', {
 })
 
 clipboard.on('success', (event) => {
+  const errorElement = document.getElementById('copy-error-callout')
+  if (errorElement) {
+    errorElement.classList.add('d-none')
+  }
+
   const iconFirstChild = event.trigger.querySelector('.bi').firstElementChild
   const namespace = 'http://www.w3.org/1999/xlink'
   const originalXhref = iconFirstChild.getAttributeNS(namespace, 'href')
@@ -39,7 +49,7 @@ clipboard.on('success', (event) => {
   }, 2000)
 })
 
-/*clipboard.on('error', () => {
+clipboard.on('error', () => {
   const modifierKey = /mac/i.test(navigator.userAgent) ? '\u2318' : 'Ctrl-'
   const fallbackMsg = 'Press ' + modifierKey + 'C to copy'
   const errorElement = document.getElementById('copy-error-callout')
@@ -49,5 +59,5 @@ clipboard.on('success', (event) => {
   }
 
   errorElement.classList.remove('d-none')
-  errorElement.insertAdjacentHTML('afterbegin', fallbackMsg)
-})*/
+  errorElement.innerHTML = fallbackMsg
+})
diff --git a/src/assets/scss/_bootstrap.scss b/src/assets/scss/_bootstrap.scss
index 4b552824..ebd53d82 100644
--- a/src/assets/scss/_bootstrap.scss
+++ b/src/assets/scss/_bootstrap.scss
@@ -13,6 +13,7 @@
 @import "bootstrap/grid";
 @import "bootstrap/tables";
 // @import "bootstrap/forms";
+@import "bootstrap/alert";
 @import "bootstrap/buttons";
 @import "bootstrap/transitions";
 @import "bootstrap/dropdown";

Copy link
Member Author

Choose a reason for hiding this comment

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

We actually can just include the bundle. Can you take care of it please? Otherwise I'll have a look later and backport the related code from the bootstrap repo.

Copy link
Member

Choose a reason for hiding this comment

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

OK I'll include (I'll try tonight) the bundle and handle it like we already do in our main repo docs.

Copy link
Member

Choose a reason for hiding this comment

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

Tried something in a37fb0f. I let you check that and tell me your thoughts.

@XhmikosR
Copy link
Member Author

XhmikosR commented Dec 1, 2022

I think it works quite well! One thing I notice is that the icon changes after 2 seconds but the tooltip stays. Maybe we should change the icon when the tooltip is hidden?

Just thinking out loud here.

element.insertAdjacentHTML('beforebegin', btnHtml)
})

window.addEventListener('load', () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

Just wondering, shouldn't we use DOMContentLoaded instead? My guess is that due to async loading we need to wait, but I'm thinking how we can improve this later.

Copy link
Member

Choose a reason for hiding this comment

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

Yes I was hesitating between both of them. IDK what's the best optimization.

Copy link
Member Author

Choose a reason for hiding this comment

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

The best optimization is to not block anything unless it's needed. So, in this case DOMContentLoaded seems a better choice.

Later, we could look into just building the Bootstrap parts we need and thus we probably won't need this code at all.

@julien-deramond
Copy link
Member

I think it works quite well! One thing I notice is that the icon changes after 2 seconds but the tooltip stays. Maybe we should change the icon when the tooltip is hidden?

Just thinking out loud here.

Thinking out loud here as well but the 2 seconds rendering modification seems pretty arbitrary. As you said, it would make more sense if the change of icon was in sync with the disappearance of the tooltip. The tooltip can't be removed automatically for a11y purpose so changing the icon when the tooltip is hidden could be OK. Maybe I can try to make the modification directly in our main repo and report it here if everyone agrees in the core team.

@XhmikosR XhmikosR force-pushed the xmr/copy branch 2 times, most recently from 82ff91d to c33415f Compare December 4, 2022 04:53
@XhmikosR
Copy link
Member Author

XhmikosR commented Dec 4, 2022

I think it works quite well! One thing I notice is that the icon changes after 2 seconds but the tooltip stays. Maybe we should change the icon when the tooltip is hidden?
Just thinking out loud here.

Thinking out loud here as well but the 2 seconds rendering modification seems pretty arbitrary. As you said, it would make more sense if the change of icon was in sync with the disappearance of the tooltip. The tooltip can't be removed automatically for a11y purpose so changing the icon when the tooltip is hidden could be OK. Maybe I can try to make the modification directly in our main repo and report it here if everyone agrees in the core team.

I'd make the change here since that's where we noticed it and we backport any fixes later upstream and in the icons repo.

XhmikosR and others added 3 commits March 16, 2023 18:09
Note that I explicitly trim the text otherwise Firefox adds a trailing EOF line feed.
@XhmikosR
Copy link
Member Author

@julien-deramond should we land this? It seems to work AFAICT even if not perfect :)

@julien-deramond julien-deramond self-requested a review March 16, 2023 20:40
Copy link
Member

@julien-deramond julien-deramond left a comment

Choose a reason for hiding this comment

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

This version is probably not perfect but is a good feature for folks. So IMHO let's merge it as-is and re-work it if we have time soon.

FYI I've just updated the branch in order to have a working version in both light and dark modes via bff52b8. I let you double-check the rendering.

@XhmikosR XhmikosR merged commit b5475d7 into main Mar 17, 2023
@XhmikosR XhmikosR deleted the xmr/copy branch March 17, 2023 08:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants