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

Add support for step, curve, and line links. Issue #174 #194

Merged
merged 1 commit into from
Nov 13, 2017

Conversation

techniq
Copy link
Collaborator

@techniq techniq commented Nov 9, 2017

🚀 Enhancements

Adds new Step, Curve, and Line links for cartesian (vertical and horizontal) and radial/polar, while retaining the original links ("diagonal") as is (no renaming).

See this codesandbox for examples. We could also update our tree example to allow switching types or possibly create a few new examples. See issue #174 for possible inspirations as well.

I plan to still implement an Elbow type as well (see this example and the elbow example of react-annotation) but it probably won't be soon-ish.

const dx = tx - sx;
const dy = ty - sy;
const ix = 0.2 * (dx + dy);
const iy = 0.2 * (dy - dx);
Copy link
Member

Choose a reason for hiding this comment

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

should the 0.2 be configurable here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I played around with this but I didn't see much benefit as this is being built using cubic bezier curves (C path command) that I ported from Vega. We would probably need to expose both of these 0.2 separately, but I wasn't sure what to call them, etc.

I took a quick stab at using a simpler quadratic curve (Q path command) where a single "midpoint" would define the curve but I was having trouble creating the same curve.

I've kicked around the idea in my head a bit and part of me thinks exposing <HorizontalLink curve={curve} />, <VerticalLink curve={curve} /> and <RadialLink curve={curve} /> and allow passing the curve function similar to show d3-shape does this. This is basically what my line functions are doing, such as...

const step = (source, target) => {
    const sa = x(source) - Math.PI / 2;
    const sr = y(source);
    const ta = x(target) - Math.PI / 2;
    const tr = y(target);

    const sc = Math.cos(sa);
    const ss = Math.sin(sa);
    const tc = Math.cos(ta);
    const ts = Math.sin(ta);
    const sf = Math.abs(ta - sa) > Math.PI ? ta <= sa : ta > sa;
    
    return `
      M${sr * sc},${sr * ss}
      A${sr},${sr} 0 0,${sf ? 1 : 0}
      ${sr * tc},${sr * ts}
      L${tr * tc},${tr * ts}
    `;
  };

I haven't looked to much into the details on how to implement this (looks like you you're supposed to use the custom curves api). This might also be overkill for 2 points...

I still think exposing the simple LinkHorizontalStep is the way to go, this is more for the customization beyond the defaults...

export { default as LinkRadial } from './shapes/LinkRadial';
export { default as LinkHorizontal } from './shapes/link/diagonal/LinkHorizontal';
export { default as LinkVertical } from './shapes/link/diagonal/LinkVertical';
export { default as LinkRadial } from './shapes/link/diagonal/LinkRadial';
Copy link
Member

Choose a reason for hiding this comment

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

Will probably need to export ./shapes/link/diagonal/{LinkHorizontal, LinkVertical, LinkRadial} from ./shapes/{LinkHorizontal, LinkVertical, LinkRadial} else it's a breaking change for folks deep linking to the components

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tried to fix this but was running into problems (I don't know if it was from Lerna or something else).

For example, I recreated src/shapes/LinkHorizontal.js and added export * from './link/diagonal/LinkHorizontal'; to re-export from this location, but when I tried up update the reference in vx-demo tree.js from

import { LinkHorizontal } from '@vx/shape';

to

import LinkHorizontal from '@vx/shape/shapes/LinkHorizontal'

I'm getting a dependency error.

image

I tried running npm run build within vx-shape and restarting npm run dev on `vx-demo.

Any ideas?

Copy link
Member

@hshoff hshoff Nov 13, 2017

Choose a reason for hiding this comment

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

does this work?

// src/shapes/LinkHorizontal.js
export { default as LinkHorizontal } from './shapes/link/diagonal/LinkHorizontal';

then run npm run build in vx-shape again

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It does not.

After that change, I also tried importing using

import LinkHorizontal from '@vx/shape/shapes/LinkHorizontal'

and

import LinkHorizontal from '@vx/shape/LinkHorizontal'

Copy link
Member

Choose a reason for hiding this comment

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

No worries, i'll take a look after merging.

@hshoff hshoff merged commit fb39f0c into airbnb:master Nov 13, 2017
@hshoff hshoff added this to the v0.0.147 milestone Nov 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants