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

Feat: implements #127 #128

Merged
merged 3 commits into from
Apr 2, 2022
Merged

Feat: implements #127 #128

merged 3 commits into from
Apr 2, 2022

Conversation

SukkaW
Copy link
Contributor

@SukkaW SukkaW commented Dec 9, 2020

I create a draft PR since It is just a Proof of Concept for #127. The PR is meant to allow for discussion and showing some refactoring issues going on.

@SukkaW
Copy link
Contributor Author

SukkaW commented Dec 9, 2020

@Scrum Also, what do you think about add tree.oncontent, tree.onattrs and tree.onnode to posthtml?

@Scrum Scrum requested a review from maltsev April 6, 2021 06:03
@maltsev
Copy link
Member

maltsev commented Apr 8, 2021

I quickly skimmed the code. I like this new approach! Let me take a more detailed look over the weekend.

@SukkaW did you do any speed benchmarks?

@SukkaW
Copy link
Contributor Author

SukkaW commented Apr 8, 2021

@SukkaW did you do any speed benchmarks?

Not "official" and correct benchmark approach, only tested on my own website, and it is indeed faster.

It will be better if we have:

  • Discuss a better API design (better naming, etc.)
  • An Official API provided by PostHTML that we can utilize.

Copy link
Member

@maltsev maltsev left a comment

Choose a reason for hiding this comment

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

An Official API provided by PostHTML that we can utilize.

Which one? Did they add something new in the recent releases?


I really appreciate you did such a big refactoring! It's really nice having such great contributors :-)

lib/htmlnano.es6 Outdated Show resolved Hide resolved
lib/htmlnano.es6 Outdated Show resolved Hide resolved
lib/htmlnano.es6 Outdated Show resolved Hide resolved
@netlify
Copy link

netlify bot commented Feb 27, 2022

‼️ Deploy request for htmlnano rejected.

Name Link
🔨 Latest commit 69637b2

@SukkaW
Copy link
Contributor Author

SukkaW commented Feb 27, 2022

@maltsev

I pick up the PR, rewrite it, and make it ready for review.

And here are the changes after the rewrite:

  • Use camelCase for named export: onAttrs, onNode, onContent instead of previous onattrs, onnode and oncontent.
  • The new module format no longer requires a once = true named export.

@SukkaW SukkaW marked this pull request as ready for review March 31, 2022 18:02
@SukkaW
Copy link
Contributor Author

SukkaW commented Mar 31, 2022

@maltsev I forgot to mark the PR as ready for review.

@maltsev
Copy link
Member

maltsev commented Apr 2, 2022

Thanks!

@SukkaW Unfortunately, I don't have time in the near future to do the proper review. But all your previous contributions were of top-notch quality. So I'm fine with just accepting this PR and releasing a new htmlnano version. Are you okay with that?

@SukkaW
Copy link
Contributor Author

SukkaW commented Apr 2, 2022

Thanks!

@SukkaW Unfortunately, I don't have time in the near future to do the proper review. But all your previous contributions were of top-notch quality. So I'm fine with just accepting this PR and releasing a new htmlnano version. Are you okay with that?

Actually, I have already tested the PR on my own project (published as @sukkaw/htmlnano) and it works fine for me. The PR shouldn't cause issues.

Also, the fix for #180 (#181) should be included in a new version, too.

@maltsev
Copy link
Member

maltsev commented Apr 2, 2022

Awesome! I'm going to merge it now then. I'll release the new version in the coming days.

@maltsev maltsev merged commit c4d7893 into posthtml:master Apr 2, 2022
@maltsev
Copy link
Member

maltsev commented Apr 4, 2022

I released the new version.

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