docs: change to/add copy code icons

[ghost]

I've added the first run of 2 copy code icons to the Getting Started page. The Clipboard API doesn't seem to work locally but I imagine will work when we push the changes live. Here is a codepen of the changes for reference as well:

https://codepen.io/christinavoudouris/pen/oNWqYLK

If these look good, I can go ahead and add them throughout the site.

[warrenrodrigues]

I see a couple of issues with this:

1. The event listener is initialized after the first click, so the copy starts working only from the second click onwards.
2. The ClipboardAPI requires additional user permission when readText is used. Your code seems to work without this line
   navigator.clipboard.readText(copiedText[i].innerText);
   Do we need this for something that perhaps I'm missing?

[ghost]

Thanks @warrenrodrigues! You're right about the readText(), it doesn't seem to be necessary. I've also wrapped everything in another event listener. See the updated Codepen.

[warrenrodrigues]

Thanks. Had a look and forked it with further updates here - https://codepen.io/warrenrodrigues/pen/ZEKoLWP
Since you're using an event listener, we don't need the onclick="copyText()" on each element. And we can also change the copyText function in JS to an anonymous function.

[ghost]

Good call! I've updated the code in the repo and moved the styles into the larger sass file as well.

[warrenrodrigues]

Thank you for all your work @christinavoudouris

I have found a couple of other things that need fixing.

• The copy button throws a Cannot read property 'innerText' of undefined error. This is because the <code> tag has an id, while the JS is querying by class.
• You have id="copiedText" multiple times in each file. As per specs, an ID can only be used once per page. So instead of
  <code class="language-javascript" id="copiedText">
  we can do
  <code class="language-javascript copiedText">

This change fixes the undefined error too.

That said, I would like someone else's opinion on whether we can use HTML code between the <pre> and <code> tags. I would rather enclose the whole thing in a <div> tag, like this

<div>
  <span class="copyMessage">Copied!</span>
  <i class="material-icons copyButton">content_copy</i>
  <pre><code class="language-javascript copiedText">
var instance = M.Carousel.getInstance(elem);
  </code></pre>
</div>

[ghost]

Oh, I must've grabbed the older code by mistake, in regards to the ID. This was changed to class a while ago. I did notice that error but I assumed it was because I was viewing the docs locally.

I'm open to changing the code a bit otherwise.

[ghost]

@warrenrodrigues What would be gained by adding a div tag around the snippet? From what I'm seeing it creates a bit more code. Would it fix a cross-browser issue or do you think it just makes the code more readable? As a React dev, I try to use Fragment as much as possible so I don't create more divs for no reason.

I've rebased my commits with the class now in place so this PR should be easier to review thus far. I'm going to hold off on adding more buttons until we resolve this so I don't have to do a massive find/replace again.

[warrenrodrigues]

Thank you for the updates @christinavoudouris

Generally, I would also avoid adding extra <div>s. However, from what I understand, the <pre> element represents a block of preformatted text. It maintains spaces and new lines, unlike how all other HTML tags behave. So I think it's best to leave the <pre> container for the actual code only. Would love to hear your opinions on this @DanielRuf @ChildishGiant

Yes, best to hold off on further updates, till this is finalized.

[ghost]

@warrenrodrigues Yeah, I ended up adding an additional inline style for the pre containers I put the button in so there's no extra padding. Wasn't sure how to go about this within the stylesheet considering the :before and :after. Any tips there are appreciated.

I also opted to leave buttons off for very short examples like instance.open(), etc.

[DanielRuf]

Hi @warrenrodrigues @christinavoudouris,

what is the current status of this PR?

Is this still wip / draft or can it be reviewed?

[ghost]

Hey @DanielRuf and @warrenrodrigues just checking in ... what's the consensus on using pre vs. divs? Has anyone else given an opinion? I am open to either. If we can come to a decision I can put in the rest of the icons and we can finally merge this thing.

[DanielRuf]

Hi @christinavoudouris,

so far I have used <pre><code... when something should be displayed / marked as preformatted code so I think this is better than <div>.

[warrenrodrigues]

Hi @DanielRuf

Please see this comment
It's about including the copy button and "Copied!" message within the <pre> tag, which I think should be avoided. I think <pre><code... should be reserved only for the actual code block; not for the copy button and message.

Does this make sense?

[DanielRuf]

Does this make sense?

That said, I would like someone else's opinion on whether we can use HTML code between the <pre> and <code> tags. I would rather enclose the whole thing in a <div> tag, like this

Thanks, yes we should wrap them in a div.

[ghost]

There's a problem with wrapping everything in a div - the copy code button doesn't show up inside the box at all. When I tried it out outside of the Materialize repo it also ruins the functionality for multiple buttons.

If I can see a working example of what you're proposing @warrenrodrigues (or @DanielRuf) I'm open to changing the code, but as of now the current code works well.

[warrenrodrigues]

Hi @christinavoudouris
Here's a codepen: https://codepen.io/warrenrodrigues/pen/yLXyWoG
Everything seems to be working fine, including multiple buttons.

[RamonvdW]

Hi, The code makes 3 queries and then depends on copyButton[i] belonging exactly to copiedText[i]. Is there any guarantee (from Javascript) that the array indexes of these 3 queries are in the same order? A small typo in the class name could cause a shift in these arrays and you end up copying the wrong text. How to find such issues? Possible solution: find the element with parent/child references. What are your thoughts on this? Ramon

…

On Thu, Aug 26, 2021 at 6:44 AM Warren Rodrigues ***@***.***> wrote: Hi @christinavoudouris <https://github.com/christinavoudouris> Here's a codepen: https://codepen.io/warrenrodrigues/pen/yLXyWoG Everything seems to be working fine, including multiple buttons. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#161 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AMBUZHV6H2C4MKBONGJPRKTT6XBEPANCNFSM5BHZYZDA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email> .

[DanielRuf]

Is there any guarantee (from Javascript) that the array indexes of these 3 queries are in the same order?

Afaik JavaScript gets them by the order in the DOM / sourcecode so this should be fine.
But iteratinc over .codeSample might be better.

[warrenrodrigues]

Possible solution: find the element with parent/child references.

Personally, this is the way I would go as well.

A small typo in the class name could cause a shift in these arrays and you end up copying the wrong text.

I think it makes sense to add only the <div class="codeSample"> ... </div> wrap around the <pre><code>... blocks on all pages. Then inject the button and message via Javascript. This way, there is less code to maintain, fix and update in multiple files.
Also, the copy button depends on Javascript to be copied, so users with Javascript disabled won't see the button at all, which is fine.

[warrenrodrigues]

Here's a codepen, with minimal HTML changes and injecting copy button and message using JS.
https://codepen.io/warrenrodrigues/pen/KKqpgvm

[RamonvdW]

Nice! Ramon

…

On Thu, Aug 26, 2021 at 8:21 PM Warren Rodrigues ***@***.***> wrote: Here's a codepen, with minimal HTML changes and injecting copy button and message using JS. https://codepen.io/warrenrodrigues/pen/KKqpgvm — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#161 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AMBUZHT7HAHHVVKEAO63SM3T62A3HANCNFSM5BHZYZDA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email> .

[DanielRuf]

Here's a codepen, with minimal HTML changes and injecting copy button and message using JS.

That looks good to me. I only see a small problem. When clicking many times in a short amount of time the events get stacked and fire one after another.

We may have to debounce it, for example with a rAF (requestAnimationFrame) implementation. See for example https://gomakethings.com/debouncing-your-javascript-events/ or by removing the pointer-events with pointer-events: none and remove this CSS rule after the 2 seconds have passed.

[DanielRuf]

We may have to debounce it, for example with a rAF (requestAnimationFrame) implementation. See for example gomakethings.com/debouncing-your-javascript-events or by removing the pointer-events with pointer-events: none and remove this CSS rule after the 2 seconds have passed.

After this is resolved we can add this code and bring the merge request to the finish line =)

[warrenrodrigues]

We may have to debounce it, for example with a rAF (requestAnimationFrame) implementation. See for example https://gomakethings.com/debouncing-your-javascript-events/ or by removing the pointer-events with pointer-events: none and remove this CSS rule after the 2 seconds have passed.

Updated codepen debouncing with pointer-events CSS.

@christinavoudouris if you're okay with these changes, feel free to push the updates to the required files.

[ghost]

@warrenrodrigues @DanielRuf Thanks guys. I feel like I've contributed a lot here and if there are going to be this many changes, feel free to reopen a new PR and take over. Might be worth just pushing what we have since at least some of the buttons work, and then do the rest.

[DanielRuf]

@warrenrodrigues @DanielRuf Thanks guys. I feel like I've contributed a lot here and if there are going to be this many changes, feel free to reopen a new PR and take over. Might be worth just pushing what we have since at least some of the buttons work, and then do the rest.

You definitely rocked this, thank you very much for the great work. I think we can make a small cut here and merge this as is.

[DanielRuf]

Does someone want to make a follow up PR? That would be awesome. @materializecss/members