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

Added in copy button #1073

Open
wants to merge 9 commits into
base: gh-pages
Choose a base branch
from
Open

Conversation

Yash-Singh1
Copy link

@Yash-Singh1 Yash-Singh1 commented Aug 24, 2020

Overview

Added in a button called Copy Result with an ID of copy-btn allowing you to copy the result of the minification process.

List of Updates

Here is a list of the updates:

  • Added in a button inside index.html with the same CSS as the earlier minify-btn
  • Enabled the copy-btn on click of the minify-btn
  • Copied the result on click of the copy-btn

Testing

To test my changes inside a development environment, go to: https://yash-singh1.github.io/html-minifier/

assets/master.js Outdated Show resolved Hide resolved
assets/master.js Outdated Show resolved Hide resolved
assets/master.js Outdated Show resolved Hide resolved
@Yash-Singh1
Copy link
Author

Any updates on this pull request, or is it good to go?

@DanielRuf
Copy link

LGTM

@Yash-Singh1
Copy link
Author

So, will @kangax merge it?

@kangax
Copy link
Owner

kangax commented Sep 21, 2020

hm, Travis CI doesn't let me merge, says some checks haven't completed yet

@Yash-Singh1
Copy link
Author

Yash-Singh1 commented Sep 23, 2020

@kangax Looks like it says that here too:
Screenshot from 2020-09-22 17-53-34
Look at this:
https://travis-ci.community/t/known-issue-travis-ci-reports-expected-waiting-for-status-to-be-reported-on-the-github-status-api-but-the-status-never-arrives/1154
I think it is a fix. Looks like this was an issue: travis-ci/travis-ci#10187
Here is another write-up someone wrote on the fix: travis-ci/travis-ci#10204
I think the solution is just to make sure that you have access to merge this pull request even when the tests haven't passed. This should be enabled in scenarios in which you want to force merge. I ran the tests manually, here is what appeared:
Screenshot from 2020-09-22 18-03-13
If you want a close up of the results:
Screenshot from 2020-09-22 18-03-54
Looks like all the tests worked manually. 👍

@Yash-Singh1
Copy link
Author

Since html-minifier now uses Gh Actions, can this be merged?

@kangax
Copy link
Owner

kangax commented Oct 8, 2021

yeah, I'm ok with it

@XhmikosR any objections?

@XhmikosR
Copy link
Collaborator

XhmikosR commented Oct 8, 2021

Yeah, this shouldn't land as is. innerHTML is meaningless when we only need to change the text.

Also, I believe we should keep the initial text instead of re-adding it.

@DanielRuf
Copy link

Yeah, this shouldn't land as is. innerHTML is meaningless when we only need to change the text.

You are right, innerText should be sufficient then.

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.

4 participants