uber / react-vis

Data Visualization Components
https://uber.github.io/react-vis
MIT License
8.73k stars 835 forks source link

Thoughts on viewBox property #901

Open dicardopegb opened 6 years ago

dicardopegb commented 6 years ago

I've been experimenting with react-vis components library and modifying it to pass a viewBox property all the way down to the svg element in XYPlot.

The results so far have been quite promising:

So my question is: why is it that this doesn't have support out of the box? Is there any drawbacks I'm not noticing.

Thanks.

mcnuttandrew commented 6 years ago

I don't quite get the particular utility of having viewbox property? I'm not opposed to it, I just don't quite get it? Can you provide some code+picture examples?

Relatedly, I've been wondering if the plot and series objects should pick up a new prop that mirrors the style prop, called svgProps which would enable the user to directly specify svg properties whereever they like.

dicardopegb commented 6 years ago

Hi @mcnuttandrew sorry for the delay, I was on vacation. I can provide an example that shows how viewBox helps with scaling and layout, just give me some minutes. Regarding your svgProps proposal I think that will be perfect actually.

dicardopegb commented 6 years ago

Here you can see an example.

Basically the sunbursts at the bottom are using the viewBox prop that I exposed in my react-vis fork. You'll notice that I can specify the coordinates of the center detail area without referring to any calculated coordinates, I'm just delegating that to the internal SVG rendering logic. So that way the SVG is scaling normally when I shrink or expand the viewport and also the circle remains centered.

I suppose you can achieve the same effect by using the flexible wrapper, but I found this method to be quicker and easier, and it also plays well with relative layout (it's easier to specify the the internal circle should be centered respective to the SVG view box).

In any case exposing the viewBox prop or the svgProps as you say shouldn't hurt right?

mcnuttandrew commented 6 years ago

@dicardopegb that link appears to be empty?

dicardopegb commented 6 years ago

Oops, so sorry. Updated now. You can just execute yarn start to test it.

mcnuttandrew commented 6 years ago

Well fair enough. I'm not going to be available to add svgProps to the library for a while, but I certainly would accept a PR that did so. Such a PR would probably need to modify XYPlot and AbstractSeries, and then probably add a number of hooks to individual data rows (eg the way LabelSeries smashes style in svg properties for each row). In addition it would need all the good hygine stuff, like docs and tests.

Caveat empetor, I feel like this might be a kinda large PR.

dicardopegb commented 6 years ago

@mcnuttandrew thanks. I'll try to work on it when I'm free, I just wanted to know if the community ever put any thought on it and discarded it for some reason.

dvjay commented 4 years ago

I need viewBox property! It could be very useful in my use-case. Infact viewBox property would have been one of the most important properties. I tried using D3 to set the viewBox but somehow it's not working. Any leads?