chrvadala / react-svg-pan-zoom

:eyes: A React component that adds pan and zoom features to SVG
https://chrvadala.github.io/react-svg-pan-zoom/
MIT License
681 stars 127 forks source link

loading a svg via file #130

Closed nufaylr closed 5 years ago

nufaylr commented 5 years ago

Hi, is it possible to load a svg via file (exported one from Adobe illustrator) ? or either using dangerouslySetInnerHTML..

nufaylr commented 5 years ago

implemented under #131 example : can use svgContain prop type

<ReactSVGPanZoom
          style={{border: "1px solid black"}}....  svgContain={props.SvgContent}>
            <svg width={800} height={800} xmlns="http://www.w3.org/2000/svg" xlinkHref="http://www.w3.org/1999/xlink" />
 </ReactSVGPanZoom>
levaifelasticad commented 5 years ago

Hi, there is any chance to merge this in master ?

nufaylr commented 5 years ago

Hi, pull request is currently pending with author. but can use my repository instead here

levaifelasticad commented 5 years ago

Thanks. Sorry, but I'm new in React. How can I use your version in my production ? I added the project with yarn, but the code is from master branch. If I modify the viewer.js the min file remains the same. Somehow I have to build it, but I don't know how to do this.

nufaylr commented 5 years ago

Yes or you can install via from github pull request using npm or yarn

using npm npm install <user>/<repo>#pull/<id>/head

yarn yarn add <user>/<repo>#<id>/head

example npm install --save-dev github:chrvadala/react-svg-pan-zoom#pull/131/head

i haven't tested the yarn command, try and let me know. yarn add chrvadala/react-svg-pan-zoom#pull/131/head

levaifelasticad commented 5 years ago

Thanks a lot. It works with yarn command.

levaifelasticad commented 5 years ago

I still have problems. After I add with yarn your version, I have this : Module not found: Error: Can't resolve 'react-svg-pan-zoom' when I use import { ReactSVGPanZoom } from 'react-svg-pan-zoom'.

levaifelasticad commented 5 years ago

Unable to resolve path to module 'react-svg-pan-zoom'.eslint(import/no-unresolved) Missing file extension for "react-svg-pan-zoom"eslint(import/extensions) module "/Users/.../Library/Caches/typescript/3.2/node_modules/@types/react-svg-pan-zoom/index"

nufaylr commented 5 years ago

I think it's looking for "main": "./build-commonjs/index.js" in the package.json. so when we take from github its not running the build commands. also it needs typescript types ? can find here @types/react-svg-pan-zoom

nufaylr commented 5 years ago

😞not the correct solution but try to install from my build branch npm install github:nufaylr/react-svg-pan-zoom#npmbuild or yarn add

levaifelasticad commented 5 years ago

Warning: Failed prop type: ReactSVGPanZoom should have a single child of the following types: svg. I use this code: render() { return (

        </div>
    );
}

where this.props.svgString is a string containing an svg like "<svg ...>"

nufaylr commented 5 years ago

SVG file can be called via Ajax. if its a class component then call under componentDidMount() lifecycle method.

Ajax example :

fetch('your-file-path/foo.svg').then(r => r.text()).then((svg)=>{
     // svg = file contain
}) 

Render example :

<ReactSVGPanZoom
          svgContain={this.props.svgString} width={500}
          height={500}
          detectAutoPan={false}
          toolbarPosition={'left'}
          miniaturePosition={'left'}>

            <svg width={500} height={500} xmlns="http://www.w3.org/2000/svg" xlinkHref="http://www.w3.org/1999/xlink" />

 </ReactSVGPanZoom>

Make sure to have an empty <svg width={500} height={500} ..../> block, svg file fetch call will be async and ReactSVGPanZoom will be expecting a single child inside.

levaifelasticad commented 5 years ago

Hi again! I added your builded version and it's working in my project with my local version. But how I can use it in production ? I'm not sure if I understand exactly the flow. If I add with yarn the project yarn add react-svg-pan-zoom it will add to my project the owner's version from npm register, and also make a build of the project. But if I add it from your github repository , the build doesn't start. So, what should I do to have a built version from your github repository and to use it in our production ?

levaifelasticad commented 5 years ago

What will happen when yarn will update the packages ? It will update from the base project ?

chrvadala commented 5 years ago

Hi guys, sorry for my late answer. I understand that load a SVG dynamically is a quite common requirement. I had also some other issues here https://github.com/chrvadala/react-svg-pan-zoom/issues/44 and here https://github.com/chrvadala/react-svg-pan-zoom/issues/116

I watched the solution proposed by nufaylr in his PR https://github.com/chrvadala/react-svg-pan-zoom/pull/131 and I think that isn't so safe to use dangerouslySetInnerHTML in a component like react-svg-pan-zoom, that is used by a lot of users. Someone could say that I may document well it, but in the end who read the documentation? 😄 I'm concerned about users that may use that feature and expose their project to XSS attack (as documented here https://reactjs.org/docs/dom-elements.html#dangerouslysetinnerhtml).

A better way could be to parse the SVG and include it in a safer way. My idea is to have something like this (or similar)

<ReactSvgPanZoomLoader src="image.svg">
{ ({width, height, content}) => (
       <ReactSVGPanZoom width={500} height={500}>
             <svg width={width} height={height}>
                  {content}
             </svg>  
       </ReactSVGPanZoom>
)
</ReactSvgPanZoomLoader>

Unfortunately actually the only available way is https://github.com/chrvadala/react-svg-pan-zoom/issues/44#issuecomment-310012989 (that uses the dangerous React feature)

nufaylr commented 5 years ago

Hi Christian, Thank you for making this library :) . Yes i do agree dangerouslySetInnerHTML it self dangerous. and your suggestion idea looks safe.. just a thought maybe we might stuck in with parsing SVG, usually SVG dom tree is massive and styles etc..

I'm happy to run a test on your idea. i will update the ticket soon.

levaifelasticad commented 5 years ago

Hi guys, Thanks a lot, with #44 it's working now.

chrvadala commented 5 years ago

Hi guys, I found this interesting post https://stackoverflow.com/a/5451238 about SVG inclusion. Tomorrow I'll perform some experiments, because I agree with nufaylr that parsing could be challenging. I hope that we'll find a good solution :)

nufaylr commented 5 years ago

Hi Christian, I was exploring this. By adding as inclusion under image src will lose the SVG tree instead will be loaded as file. in my case i wanted to attach some event listeners to some group tags etc,.. also xlink:href is deprecated can be used as href instead.

Definitely needs do more investigation on this, maybe we can find a workaround..

chrvadala commented 5 years ago

Can I ask you, how are you adding that extra listener on those nodes? Are you using plain javascript or the React way?

nufaylr commented 5 years ago

Currently doing in a plan Javascript way 😞, but i wanted to move into React way. I came across this module react-svgmt. Looks promising and SVG parsing.

In Addition your suggested idea might need to change bit like this :

<ReactSvgPanZoomLoader src="image.svg" render= {(width, height, content) => (
    <ReactSVGPanZoom width={500} height={500}>
             <svg width={width} height={height}>
                  {content}
             </svg>  
     </ReactSVGPanZoom>
)}/>

Some resources render-props , FaCCs as antipattern

Open for suggestions ???

nufaylr commented 5 years ago

Hi guys, I have investigate into react-svgmt module, and it use inline SVGInjector library with solid svg phaser, thought of not to reinvent the wheel instead use react-svgmt module. see the PR #135 and open for suggestions.

Example on how to use here :

 import {ReactSVGPanZoom, ReactSvgPanZoomLoader, SvgLoaderSelectElement} from 'react-svg-pan-zoom'

Example 1 :

<ReactSvgPanZoomLoader src="image.svg" render= {(content) => (
    <ReactSVGPanZoom width={500} height={500}>
        <svg width={500} height={500}>
            {content}
        </svg>  
     </ReactSVGPanZoom>
)}/>

Example 2 with proxy node prop type, under here we can target SvgLoaderSelectElement component.

<ReactSvgPanZoomLoader src="image.svg" proxy = {

       <SvgLoaderSelectElement selector="#builtstructure" onClick={onItemClick} stroke={props.strokeColor}/>
       <SvgLoaderSelectElement selector="#tree" onClick={onItemClick} />

  } render= {(content) => (

    <ReactSVGPanZoom width={500} height={500}>
             <svg width={500} height={500}>
                  {content}
             </svg>  
     </ReactSVGPanZoom>

)}/>
chrvadala commented 5 years ago

Thanks for your PR. Since this implementation requires an external dependency I was wondering if it is better to release it as a different package. Thought?

nufaylr commented 5 years ago

Yes sounds good. So Adding as packages folder in the root?

chrvadala commented 5 years ago

I was evaluating three different options:

  1. Restructure the entire repo and create two folders /react-svg-pan-zoom and /react-svg-pan-zoom-loader
  2. Use Lerna
  3. Create a new repo

About Lerna I used it here https://github.com/chrvadala/react-refactor and honesty I don't like it :) For the other alternative probably create a new repo, could be the better way

nufaylr commented 5 years ago

I haven't used Lerna. I have always seen in big projects are using Lerna. In that case option 1 and 2 will be similar. option 3 (create a new repo) means publishing into NPM registry ?

chrvadala commented 5 years ago

For new repo I mean a new GitHub repo. This makes more easy to maintain this new project. I'll create this new repo late this day. If you want, I can share with you the project maintenance.

nufaylr commented 5 years ago

Yes Sounds good, Happy to get involved in.. 👍

nufaylr commented 5 years ago

Hi @chrvadala , Many thanks. I have accepted the request, it's a private repo, any reason to keep it private?

chrvadala commented 5 years ago

I'll publish it as soon as we have a first running version. Hope that isn't a problem ;-)

chrvadala commented 5 years ago

A new library has been built. It's available here https://github.com/nufaylr/react-svg-pan-zoom-loader