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

Parse5 implementation #5

Merged
merged 22 commits into from Dec 26, 2020
Merged

Parse5 implementation #5

merged 22 commits into from Dec 26, 2020

Conversation

gdbaldw
Copy link
Contributor

@gdbaldw gdbaldw commented Dec 24, 2020

Added test and demo for TDD, then refactored per @bennypowers Parse5 implementation, and expanded the configuration options, organized in three separate commits. A goal was no breaking changes, as demonstrated in the test cases and the demo. HTML imports are moved from the "src" attribute to an embedded script, which should be functionally equivalent.

@bennypowers
Copy link
Contributor

Closes #4

Really nice work, @gdbaldw

@jdvivar
Copy link
Owner

jdvivar commented Dec 24, 2020

Added test and demo for TDD, then refactored per @bennypowers Parse5 implementation, and expanded the configuration options, organized in three separate commits. A goal was no breaking changes, as demonstrated in the test cases and the demo. HTML imports are moved from the "src" attribute to an embedded script, which should be functionally equivalent.

Grrrreat! I'll take a look as soon as I can! Happy Holidays!

@jdvivar
Copy link
Owner

jdvivar commented Dec 25, 2020

@gdbaldw

  • I reaaaaaally like the tests you added, I'm so fan of TDD tests: very elegant and useful, I'll work to add CI into the project and use them once merged
  • Why you don't like index.js for a single export in a simple function package like this? I think .eleventy.js is confusing because that's the name for the Eleventy configuration file, which is not the case here. I've changed it back to index.js but please, if you think there's a good reason it should be .eleventy.js for a plugin, please tell me, I might be missing something :/
  • I also liked a lot your /sample addition, I've worked to make it a bit more complete (with one real Web Component file and all!) and now it opens live in your browser. Also I've added a reference to this new feature in the readme file. Rename it to demo instead.
  • I've added an .npmignore file to omit the demo and test folders to keep the package size to a minimum at npm

Oh and remember to use commitizen-style git messages, those are fed up to the changelog and to the engine which automatically bumps up package version numbers. I really recommend you to use them in your projects as well :) (more info: https://github.com/conventional-changelog/standard-version)

What a great improvement!

What do you think @gdbaldw @bennypowers ?

README.md Outdated Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
@gdbaldw
Copy link
Contributor Author

gdbaldw commented Dec 26, 2020

Regarding .eleventy.js vs index.js for a plugin package.json "main" attribute, I have no opinion. I followed the lead of a zachleat plugin. By the way, also copied his approach to testing and demo.

index.js Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
| `verbose` | `Boolean` | `false` | It will console log each step, for debug purposes |
| `quiet` | `Boolean` | `false` | It won't console log anything. By default, a log of each Web Component definition is log out with this format: `[add-web-component-definitions] Adding definition for tag: custom-tag`|
| `singleScript` | `Boolean` | `false` | If true, only one script with import statements will be output: `<script type="module">import "js/components/custom-tag.js;</script>` |
Copy link
Owner

Choose a reason for hiding this comment

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

What do you think about this? I've implemented both a single script with many imports and many scripts with src (default, as it was in my original plugin), added some tests and modified the previous ones with the default option.

If you're both happy, I'll merge it @gdbaldw @bennypowers :)

Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM. I wonder it it would be better to pass the tree to addChild rather than hold the body in closure, but that's a small nit.

@jdvivar jdvivar merged commit 3444700 into jdvivar:master Dec 26, 2020
@gdbaldw
Copy link
Contributor Author

gdbaldw commented Dec 27, 2020

Thank you for the compliments, and I really like the amendments. I'm in Taipei now, and so was sleeping at final review - looks great. And, glad to assist.

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.

None yet

3 participants