-
-
Notifications
You must be signed in to change notification settings - Fork 91
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
Add rehype-scroll-to-top
, rehype-smenatic-images
to list of plugins
#163
Conversation
Add new 'scroll to top' plugin
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
node16
ornodenext
mode, 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
dependencies
overpeerDependencies
, different package managers handlespeerDependencies
in different ways, they're often cause more trouble than their worth.dependencies
are 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-coverage
it can be good to have a baseline minimum (example:Lines 57 to 62 in c006fa9
"typeCoverage": { "atLeast": 100, "detail": true, "ignoreCatch": true, "strict": true }, unified
andrehype
tend 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
package-lock=false - For the
rehype-sematic-images
plugin 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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.