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

Simplify usage of selection.join with transitions #286

Merged
merged 7 commits into from
Jun 7, 2021
Merged

Conversation

curran
Copy link
Contributor

@curran curran commented May 25, 2021

Related to #257, an alternative to #285, this change would allow .join to handle the case that transitions are returned by onenter and/or onupdate. It would support the same simplified usage documented in #257, but leave the behavior of .merge unchanged. Just putting this out there as an alternative to consider, as it targets the main pain point, rather than introducing new behavior in .merge to accomplish the same goal.

This works because of selection.selection.

@curran curran changed the title Alternative solution for Alternative solution for transitions in .join May 25, 2021
@@ -3,5 +3,5 @@ export default function(onenter, onupdate, onexit) {
enter = typeof onenter === "function" ? onenter(enter) : enter.append(onenter + "");
if (onupdate != null) update = onupdate(update);
if (onexit == null) exit.remove(); else onexit(exit);
return enter && update ? enter.merge(update).order() : update;
return enter && update ? enter.selection().merge(update.selection()).order() : update;
Copy link
Member

@mbostock mbostock May 25, 2021

Choose a reason for hiding this comment

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

I think we should do this earlier so that selection.join is guaranteed to return a selection, like so:

function(onenter, onupdate, onexit) {
  var enter = this.enter(), update = this, exit = this.exit();
  if (typeof onenter === "function") {
    enter = onenter(enter);
    if (enter) enter = enter.selection();
  } else {
    enter = enter.append(onenter + "");
  }
  if (onupdate != null) {
    update = onupdate(update);
    if (update) update = update.selection();
  }
  if (onexit == null) exit.remove(); else onexit(exit);
  return enter && update ? enter.merge(update).order() : update;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yes good catch! Thank you.

@curran
Copy link
Contributor Author

curran commented May 25, 2021

Remaining work:

  • Tests
  • Docs

@curran
Copy link
Contributor Author

curran commented May 25, 2021

I just realized that if #285 lands in addition to this, then we will not need the following:

if (update) update = update.selection();

This is because the updated .merge implementation from #285 will do this internally.

@curran curran changed the title Alternative solution for transitions in .join Support returning a transition from onenter in selection.join May 25, 2021
@mbostock
Copy link
Member

I just realized that if #285 lands in addition to this, then we will not need the following:

if (update) update = update.selection();

This is because the updated .merge implementation from #285 will do this internally.

No, you’ll still want it because otherwise selection.join could return a transition if there’s no enter selection:

  return enter && update ? enter.merge(update).order() : update;

@curran curran changed the title Support returning a transition from onenter in selection.join Allow onenter and onupdate to return a transition in selection.join May 25, 2021
@curran curran marked this pull request as ready for review May 25, 2021 21:47
@curran
Copy link
Contributor Author

curran commented May 25, 2021

Added tests and docs. Ready for review. Thanks!

@curran curran changed the title Allow onenter and onupdate to return a transition in selection.join Simplify usage of selection.join with transitions May 27, 2021
@mbostock mbostock merged commit 105ecb9 into d3:main Jun 7, 2021
@curran
Copy link
Contributor Author

curran commented Jun 7, 2021

Hooray! Thank you @mbostock 🙏

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

Successfully merging this pull request may close these issues.

2 participants