fskpf / svg2roughjs

Create sketchy, hand-drawn-like images from SVGs
https://fskpf.github.io/
MIT License
158 stars 13 forks source link

Enhance `Svg2Roughjs` constructor to accept DOM node without TypeScript errors #108

Closed 87xie closed 3 months ago

87xie commented 3 months ago

Is your feature request related to a problem? Please describe.

Allowing the constructor's target parameter to accept a DOM node directly. Although passing a DOM node currently does not cause runtime errors, TypeScript raises type errors.

This enhancement would be particularly beneficial in modern front-end frameworks like React. Here's an example:

import { useRef, useEffect } from 'react';
import { Svg2Roughjs } from 'svg2roughjs';

function MyComponent() {
  const outputRef = useRef<HTMLDivElement | null>(null);
  useEffect(() => {
    if (!ref.current) return;
    const svg2roughjs = new Svg2Roughjs(outputRef.current);
    // To avoid TypeScript errors like:
    // Argument of type 'HTMLDivElement' is not assignable to parameter of type 'string | HTMLCanvasElement | SVGSVGElement'.
  }, []);

  return (
    <div ref={outputRef}>
    </div>
  );
}

Describe the solution you'd like

One simple way to accomplish this is by adjusting the type of the target param. However, I'm uncertain whether this change might have other implications. Feel free to close this if I have missed something obvious.

class Svg2Roughjs {
  // ...
  constructor(
    target: string | HTMLElement,
  ) {
  // ...
}

Additional context

N/A

ygra commented 3 months ago

What you're missing is that the library does not convert SVG to HTML, but either to SVG or a bitmap on a canvas element. So passing an HTMLDivElement would not really work. What we perhaps should do is instead of setting the backing property for outputType in line 168 to just set the property so we get the proper validation as well.

87xie commented 3 months ago

Thanks for the reply! To clarify the purpose of the issue, let's focus on the usage.

Targeting a parent HTMLElement with a string selector #output-div

const svg2roughjs = new Svg2Roughjs('#output-div');
svg2roughjs.svg = document.getElementById('input-svg');
svg2roughjs.sketch();

In some cases, we already have a DOM node reference. It would be helpful if the Svg2Roughjs instance could be created by passing the DOM node directly.

const outputDiv = document.querySelector('#output-div');
const inputSvg = document.getElementById('input-svg');

// Assuming Svg2Roughjs can accept an HTMLElement directly
const svg2roughjs = new Svg2Roughjs(outputDiv);
svg2roughjs.svg = inputSvg;
svg2roughjs.sketch();

The current implementation seems to support passing a DOM node directly, but in TypeScript, there will be type error hints.

fskpf commented 3 months ago

You are correct. The implementation works also when passing a container div, as the snippet suggests. Depending on the outputType, it appends a canvas- or svg-element (defaults to svg) to the given container (as long as it is not a canvas or svg element). I've added this to the typing of the target element.