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

selection.merge(transition) #285

Merged
merged 4 commits into from
Jun 7, 2021
Merged

selection.merge(transition) #285

merged 4 commits into from
Jun 7, 2021

Conversation

curran
Copy link
Contributor

@curran curran commented May 24, 2021

I ended up investing what it might take to implement #257 and the code change seemed relatively straightforward, so I'm submitting this draft PR as a first step towards maybe implementing the feature.

I realize the implementation is not the hard part here. The hard part is weighing the pros and cons of introducing more surface area to this API.

Pros:

  • Would simplify usage of .join with transitions (no need to use .call).
  • Backwards compatible.
  • Applies only to a "deprecated API.".

Cons:

  • Adds surface area to an API that many folks already find confusing to use, which may lead to more support effort.

Closes #257

Inspired by d3-axis.

@curran
Copy link
Contributor Author

curran commented May 24, 2021

TODO:

  • Await any and all feedback
  • Tests
  • Docs

@curran curran changed the title Implement selection.merge(transition) selection.merge(transition) May 24, 2021
Copy link
Member

@Fil Fil left a comment

Choose a reason for hiding this comment

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

Do we also want to expose transition().merge(transition), so that merge works in all cases?
If a is a transition, should a.merge(b) be a transition?

https://github.com/d3/d3-selection#selection_merge

src/selection/merge.js Outdated Show resolved Hide resolved
@mbostock
Copy link
Member

Isn’t there already a transition.merge?

Co-authored-by: Philippe Rivière <[email protected]>
@curran
Copy link
Contributor Author

curran commented May 25, 2021

Here's an alternative idea that enables the same simplification to usage of .join, but does not touch .merge: #286

src/selection/merge.js Outdated Show resolved Hide resolved
@mbostock
Copy link
Member

I think this approach (with my suggestion) is fine to do in addition to #286, if we want to be more lax. The general idea is we can be more lax with inputs, but we should be strict with outputs: selection.merge should always return a selection, even if we allow you to pass in a transition. Similar selection.join should always return a selection, even if we allow you to return a transition from the enter or update callback.

@mbostock mbostock merged commit 689a476 into d3:main Jun 7, 2021
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.

selection.merge(transition…)?
3 participants