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: rehype-remove-images #47

Closed
wants to merge 1 commit into from
Closed

Conversation

iloveitaly
Copy link

Initial checklist

  • I read the support docs
  • I read the contributing guide
  • I agree to follow the code of conduct
  • I searched issues and couldn’t find anything (or linked relevant results below)
  • If applicable, I’ve added docs and tests

Description of changes

Simple plugin to remove images from HTML.

The typing syntax feels strange compared to normal typescript, so the build is failing. Before digging further, I wanted to submit a draft PR
to see if this is something you'd like to include in the package.

If you could point me in the right direction to fix up the TS issues / understand the new TS syntax I', seeing here I'd be grateful.

@github-actions github-actions bot added the 👋 phase/new Post is being triaged automatically label Jul 7, 2023
@github-actions

This comment has been minimized.

@iloveitaly
Copy link
Author

@wooorm this PR isn't ready yet, but I'm curious if you could point me in the right direction to fix up this PR.

Copy link
Member

@wooorm wooorm left a comment

Choose a reason for hiding this comment

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

Hi there!

I wonder if this is the place to maintain this.
Typically images are semantic: they have meaning to the document.
This project performs changes that typically remain equal to the original input.

Like many other rehype plugins, I think you can maintain this separately?

As for this PR:

  • the code style doesn’t yet match the project, run npm test to solve that, it generates the readme too
  • as for type issues, I think the only thing is that you create a project that does not accept options? so you’d need * @type {import('unified').Plugin<[], Root>}
    for more on how types through JSDoc work, see https://www.typescriptlang.org/docs/handbook/jsdoc-supported-types.html, https://github.com/voxpelli/types-in-js
  • visit is much more powerful, and simpler, than remove. Here’s an example of what you want to do:
    import type {Nodes} from 'hast'
    import {visit} from 'unist-util-visit'
    
    const tree: Nodes = getNodeSomehow()
    
    visit(tree, function (node, index, parent) {
      if (parent && index !== undefined && node.type === 'element' && node.tagName === 'img') {
        parent.children.splice(index, 1)
        return index
      }
    })

@iloveitaly
Copy link
Author

@wooorm thanks for the helpful response!

Do you think I should just pull this into a separate repo, or would you be open to including it here if the issues were fixed?

@wooorm
Copy link
Member

wooorm commented Jul 15, 2023

The former, as of yet I don’t feel like it fits here, but maybe you have good reasons to say it is useful to just minify without side-effects for most folks (opt-in or not).
So, make your own repo, PR to add it to the list of plugins, and ask for a review on tips! (or feel free to open discussions earlier!)

@wooorm wooorm closed this Jul 15, 2023
@wooorm wooorm added the 👀 no/external This makes more sense somewhere else label Jul 15, 2023
@github-actions

This comment has been minimized.

@github-actions github-actions bot added 👎 phase/no Post cannot or will not be acted on and removed 👋 phase/new Post is being triaged automatically labels Jul 15, 2023
iloveitaly added a commit to iloveitaly/rehype that referenced this pull request Jul 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
👀 no/external This makes more sense somewhere else 👎 phase/no Post cannot or will not be acted on
Development

Successfully merging this pull request may close these issues.

2 participants