Skip to content

Conversation

ghost
Copy link

@ghost ghost commented Jul 23, 2021

Proposed changes

Just a quick fix to my copy code button. I added a line to disallow multiple messages, aka the user can only click the button once.

Types of changes

  • Bug fix (non-breaking change which fixes an issue).
  • New feature (non-breaking change which adds functionality).
  • Breaking change (fix or feature that would cause existing functionality to change).

Checklist:

  • I have read the CONTRIBUTING document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Copy link
Member

@Smankusors Smankusors left a comment

Choose a reason for hiding this comment

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

hmm, I'm not sure about this, how about if people want to copy two times? For example, the dev clicked this button to copy the code, but for some reason, they decided to copy and paste something else. After that, this dev wants to copy the code again by clicking this button. Then confused the button does nothing.

The better solution would be disabling the button instead of silently doing nothing. Although, I do prefer the copy button that can be pressed multiple times.😉


At the same time, I just noticed that the indentation of your code kinda little mess IMO. Can you please fix that? I prefer this style below (also don't forget the semicolon, well it's not required technically, but just make the browser easier parsing the code).

  document.getElementById('copyButton')
    .insertAdjacentHTML('afterend',
      `<span style="margin-left:10px; font-size:14px;">Copied!</span>`
    );
} 

@ghost
Copy link
Author

ghost commented Jul 23, 2021

Hey, I just reformatted the code (had some autoformatting on and working late last night; sorry about that) and was able to disable the button upon click.

I see what you're saying, but the problem before was that if you copied twice, you'd get a second "Copied!" message (or more) with each click. In terms of allowing multiple clicks, I also think because these are just CDN links, people will grab the code as-is. If the problem is the user wanting only one link, a better solution would be to create a different snippet for the JS.

@ghost
Copy link
Author

ghost commented Jul 23, 2021

I'm also going to open an issue about potentially adding more of these buttons throughout the site, so we can talk more about different implementations.

@ghost ghost requested a review from Smankusors July 23, 2021 20:36
Copy link
Member

@Smankusors Smankusors left a comment

Choose a reason for hiding this comment

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

Alright it's ok IMO, but I'm actually not sure how to proceed with this PR. Any thoughts? @materializecss/members

@DanielRuf
Copy link

I think it would be better to just show the message "code copied" and hide it again after a specific amount of time (setTimeout) or better with a css transition / animation and removing the "show" class. Clicking again will just do the same (hide and show).

Disabling the button is not ideal imho, especially UX or DX wise here.

I think this makes more sense. What do you think?

@ghost
Copy link
Author

ghost commented Jul 24, 2021

Honestly, I thought of that but it didn't work for me. That's why I opted to turn off the button initially. If anyone wants to give it a shot, feel free to hijack my code, but I don't have time to work on it today. Here's my codepen for it:

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

@warrenrodrigues
Copy link

@christinavoudouris played around with your codepen and I think this could be a decent option, re-using the same message container and hide it 2 seconds (or something of that sort), after the copy button is clicked.

https://codepen.io/warrenrodrigues/pen/VwbrNjJ

CSS can be placed inline as required.

@DanielRuf
Copy link

@christinavoudouris played around with your codepen and I think this could be a decent option, re-using the same message container and hide it 2 seconds (or something of that sort), after the copy button is clicked.

codepen.io/warrenrodrigues/pen/VwbrNjJ

CSS can be placed inline as required.

Let me know if you need support to make the changes. I think the proposed change from this codepen is an ideal solution for most users =)

@wuda-io
Copy link
Member

wuda-io commented Jul 24, 2021

@christinavoudouris played around with your codepen and I think this could be a decent option, re-using the same message container and hide it 2 seconds (or something of that sort), after the copy button is clicked.

https://codepen.io/warrenrodrigues/pen/VwbrNjJ

CSS can be placed inline as required.

Looks good to me bruh

@warrenrodrigues
Copy link

Created PR #158 for the message container issue. Also changed the method of copying the text, so that a textarea is not created at the bottom of the page.

@DanielRuf
Copy link

Thank you @warrenrodrigues for the PR. I think we can close this PR here in favor of yours.
@christinavoudouris has done great work here and I think her review for your PR with your changes could be very useful.

@DanielRuf DanielRuf closed this Jul 24, 2021
@warrenrodrigues
Copy link

Yes, definitely.

@ghost ghost deleted the copy-code branch July 24, 2021 22:36
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.

5 participants