Add rehype-scroll-to-top, rehype-smenatic-images to list of plugins#163
Add rehype-scroll-to-top, rehype-smenatic-images to list of plugins#163wooorm merged 4 commits intorehypejs:mainfrom
rehype-scroll-to-top, rehype-smenatic-images to list of plugins#163Conversation
Add new 'scroll to top' plugin
There was a problem hiding this comment.
Thanks for sharing @benjamincharity! 🙇
Always happy to have new plugins in the community!
A few suggestions, for this PR could you look into
doc/plugins.md
173:20-173:21 warning Expected a smart quote: `“`, not `"` quote retext-quotes
173:34-173:35 warning Expected a smart quote: `”`, not `"` quote retext-quotes
173:40-173:41 warning Expected a smart quote: `“`, not `"` quote retext-quotes
173:57-173:58 warning Expected a smart quote: `”`, not `"` quote retext-quotes
173:86 warning Line must be at most 80 characters maximum-line-length remark-lint
For the code base itself, some suggestions:
- Consider publishing types with
node16ornodenextmode, this ensures all the import and exports match the Node JS resolution rules which are more specific/strict than general ESNext https://www.typescriptlang.org/docs/handbook/modules/reference.html#node16-nodenext (source: https://github.com/benjamincharity/rehype-scroll-to-top/blob/58781b01c94f32bafe2074dcff6d2e324a6aa81c/tsconfig.json#L5-L8) - Object spread can be a concise way to merge objects, for example
bottomLink: {...defaultOptions.bottomLink, ...userOptions.bottomLink}(source: https://github.com/benjamincharity/rehype-scroll-to-top/blob/58781b01c94f32bafe2074dcff6d2e324a6aa81c/src/index.ts#L87-L100) - You appear to be publishing through an automated CI process, consider using provenance to make the builds more verifiable https://github.blog/2023-04-19-introducing-npm-package-provenance/ (source: https://github.com/benjamincharity/rehype-scroll-to-top/blob/58781b01c94f32bafe2074dcff6d2e324a6aa81c/.github/workflows/publish.yml#L28)
- If unpacked/destructured values are your preference, they can be unpacked in the function parameter definition to avoid extra variable allocation https://javascript.info/destructuring-assignment#smart-function-parameters (source: https://github.com/benjamincharity/rehype-scroll-to-top/blob/58781b01c94f32bafe2074dcff6d2e324a6aa81c/src/index.ts#L104-L105)
- When leveraging TypeScript it can also be helpful to test type coverage to ensure all of the API and internals are type safe https://github.com/plantain-00/type-coverage
- It can be helpful to have a linter to check conventions and best practices in code, the unified community is fond of https://github.com/xojs/xo but more custom ESLint setups can also work.
|
I am new to the rehype ecosystem, and this is my first rehype plug-in, so I appreciate the detailed response! I will look into these today. |
|
@ChristianMurphy I have released a new version of my plugin that addresses all of the listed issues ( I am seeing failures in the actions that seem unrelated to my changes. Please let me know if there is anything else I should adjust. |
There was a problem hiding this comment.
Thanks @benjamincharity!
Looks good, could you put rehype-semantic-images in alpha order in this list? Then this looks good.
Don't worry about the XO error for this PR it's unrelated.
But if you have time you're welcome to look into it in another PR, contributions are always welcome! 🙂
Optional suggestions:
- I'd recommend defining
dependenciesoverpeerDependencies, different package managers handlespeerDependenciesin different ways, they're often cause more trouble than their worth.dependenciesare handled fairly consistently across managers (source: https://github.com/benjamincharity/rehype-scroll-to-top/blob/46ab8cfb5066d05ce26e76cd89b4306227148213/package.json#L55-L58 and https://github.com/benjamincharity/rehype-semantic-images/blob/13755f0da6340e64805b49ab3560f321ac6889e0/package.json#L61-L64) - with
type-coverageit can be good to have a baseline minimum (example:)Lines 57 to 62 in c006fa9
unifiedandrehypetend to use 100%, but any greater than zero baseline is a good start. - While lockfiles are faster, they can hide issues downstream adopters may see clean installing from CI/CD by locking older versions of dependencies in place, unified/remark turn lockfiles off entirely (source: )
Line 2 in c006fa9
- For the
rehype-sematic-imagesplugin it is a bit strange that a non-semantic<div>is being injected (source: https://github.com/benjamincharity/rehype-scroll-to-top/blob/46ab8cfb5066d05ce26e76cd89b4306227148213/src/index.ts#L114) Is the<div>needed? Could a more semantic alternative be used?
ChristianMurphy
left a comment
There was a problem hiding this comment.
LGTM, thanks @benjamincharity!
Same optional comments from #163 (review) still apply, but aren't a blocker for adding this to the docs.
rehype-scroll-to-top, rehype-smenatic-images to list of plugins
This comment has been minimized.
This comment has been minimized.
|
Thanks Benjamin! |
Add new 'scroll to top' plugin
Initial checklist
Description of changes
Add a new Rehype plugin to the plugins doc.
rehype-scroll-to-top.