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

GraphNode.hasDefaultCurveNode REVIEW comment #291

Closed
pixelzoom opened this issue Mar 15, 2023 · 2 comments
Closed

GraphNode.hasDefaultCurveNode REVIEW comment #291

pixelzoom opened this issue Mar 15, 2023 · 2 comments

Comments

@pixelzoom
Copy link
Contributor

Related to #288, for discussion about this REVIEW comment in GraphNode.ts:

  // REVIEW: This naming is a bit confusing, and I first thought a GraphNode had a default curveNode
  // REVIEW: in addition to something else that was provided. Not in place of. It's unclear that the default is
  // REVIEW: a non-interactive curve, rather than a curve which also happens to be non-interactive.
  // REVIEW: perhaps just `hasInteractiveCurveNode`?
  // Whether to create the default CurveNode, which is not interactive
  hasDefaultCurveNode?: boolean;

Whether the curve is interactive is really irrelevant in GraphNode; it has no notion of "interaction". What we're trying to describe by this option is whether GraphNode should create the CurveNode that is typically created for a Curve. Would it help to rename hasDefaultCurveNode to createCurveNode, and delete the "which is not interactive" comment, like this?

  // Whether to create the default CurveNode for the provided Curve
  createCurveNode?: boolean;
@pixelzoom pixelzoom changed the title GraphNode.hasDefaultCurveNode review comment GraphNode.hasDefaultCurveNode REVIEW comment Mar 15, 2023
pixelzoom added a commit that referenced this issue Mar 15, 2023
@pixelzoom
Copy link
Contributor Author

In the above commit, I went ahead and made the change that I suggested above.

Summary, from GraphNode.ts:

  // Whether to create a CurveNode for the provided Curve.
  createCurveNode?: boolean;
...
  // Optional Node that plots the provided Curve, see SelfOptions.createCurveNode
  private readonly curveNode?: CurveNode;
...
    // Create CurveNode for the provided Curve.
    if ( options.createCurveNode ) {
      this.curveNode = new CurveNode( curve, this.chartTransform, {
...

@marlitas if this looks OK to you, please close. Otherwise let's discuss.

@marlitas
Copy link
Contributor

Yes, this is much clearer! Thank you. Closing :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants