Skip to content

Conversation

@SuperPaintman
Copy link

Good afternoon!

I changed the build system a little:

  • Removed third-party modules' files and moved them to the Node modules (vivus and dom-to-image)

Also I fixed few bugs (mistakes):

  • innerText -> textContent
  • classList -> className - because you assigned string to the classList

@octref
Copy link
Owner

octref commented Feb 21, 2018

Still, if we can just have a few kbs of minified files why have dependencies pulling everything in?

Other than that this looks good. Thanks!

@SuperPaintman
Copy link
Author

SuperPaintman commented Feb 21, 2018

Still, if we can just have a few kbs of minified files why have dependencies pulling everything in?

The first reason is that they (minified files) look like we want to hide them from users, like a virus or other "sick" code.

The second reason it that VS Code pull the dependencies on its own (on install). And there is no reason to store them in this repo.

@runofthemillgeek
Copy link

I agree with @SuperPaintman here. I thought there was magic thing going on in dom2image.js and besides that, if it gets updated in the future with new features/changes, we'd just have to flip the versions in package.json and have it up & running.

const initialSpans = doc.querySelectorAll('div > div span:first-child')
for (let i = 0; i < initialSpans.length; i++) {
initialSpans[i].innerText = initialSpans[i].innerText.slice(indent)
const initialSpan = initialSpans[i]
Copy link
Owner

Choose a reason for hiding this comment

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

just one line is fine. don't declare unnecessary variables.

@octref
Copy link
Owner

octref commented Feb 22, 2018

Well, I went for #40. This is my repo so I do what I like.

If you can limit this PR to just the textContent, className and opacity change I'll take it. Otherwise no.

@SuperPaintman
Copy link
Author

SuperPaintman commented Feb 22, 2018

Well, I went for #40. This is my repo so I do what I like.

Sure, It's totally your choice. If you want to confuse users with minified files - you can do it.

If you can limit this PR to just the textContent, className and opacity change I'll take it. Otherwise no.

Could you do it on your own?

@octref octref closed this Feb 22, 2018
octref added a commit that referenced this pull request Feb 22, 2018
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.

3 participants