-
Notifications
You must be signed in to change notification settings - Fork 161
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 .contain() method? #49
Comments
Yes, I think this is a great idea. I filed this as d3/d3-geo-projection#30 previously but that was back when I was planning on keeping the projection-related code in that module (which won’t be possible until the new geo pipeline is implemented for 5.0), so anyway, it’s fine to move that issue over here. That issue has my proposed API: path.fit(feature, extent). But I haven’t thought about it too much so it might need redesigning. |
Oh great, I hadn't seen that one. It seems to me like it would be a little more intuitive to attach it to the projection rather than the path since the projection's what's getting modified. How about something like:
Where |
You’ll need path.bounds to compute the bounding box, and potentially the Perhaps path.area, path.bounds and path.centroid should move to the
|
An extent is [[x0, y0], [x1, y1]]. A size is [dx, dy] but I wouldn’t want
to do automatic inference to distinguish them; I’d require an extent.
Unless we create a separate d3.geoFit object and then you could specify
either an extent or a size. But that feels a little overkill.
I’m not wild about the optional padding argument as I tried to remove those
in D3 4.0. It might be okay, but I think it’s equally likely you might want
a fixed size margin rather than a percentage of width and height. One
option would be helper methods which derive a new extent from an existing
extent (to scale or subtract) but that might be clunky too.
|
A separate geoExtent or geoFit object definitely feels like overkill. Skipping the padding works for me. Will probably lead to lots of code along the lines of:
In cases where the container hadn't been pre-translated. Not the neatest but it's nice and explicit and seems particularly appropriate for a canvas context or insets/small multiples. Moving |
We can move the methods in 4.2 and leave the old ones as deprecated aliases
|
A related consideration here is whether projection.fit should set I guess it should set only projection.translate, but I did find it useful But in general it might be hard to set projection.center to the unprotected
|
Actually I forgot that path.area and related methods are sometimes used
|
One thing to note, it seems like that project to bounding box approach goes haywire with a gnomonic projection: https://gist.github.com/veltman/a1d532a697764f05d0093dec40c571b2 |
Yes. Many projections extend to infinity if you’re not careful.
|
Any interest in adding a
projection.contain()
method that projects to a bounding box a la http://bl.ocks.org/mbostock/4707858?I think it's a pretty common use case to start with a bounding box rather than a desired scale/translate, and the alternative is to fiddle with values until it looks nice.
I took a crack at it, mostly seems to work except I need to figure out why it doesn't work for projections with clipExtents:
https://github.com/veltman/d3-geo/blob/master/src/projection/index.js#L81-100
In terms of naming, I chose
contain
because that's the CSSbackground-size
nomenclature, but it could also be something likefitTo
.This would break the getter/setter pattern a bit. Alternatively, it could be a more consistent separate method like:
Although that seems kind of verbose. Any thoughts?
The text was updated successfully, but these errors were encountered: