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

One #300

Closed
wants to merge 10 commits into from
Closed

One #300

wants to merge 10 commits into from

Conversation

curran
Copy link
Contributor

@curran curran commented Feb 21, 2022

Closes #294

@curran
Copy link
Contributor Author

curran commented Mar 4, 2022

Thoughts? If I add tests and docs would there be interest?

@Fil
Copy link
Member

Fil commented Mar 5, 2022

I agree that it could be a useful shorthand notation, but I'm afraid it introduces some sort of conceptual problems.

One can read the proposal as a way of extending document.querySelector from a "query" to a "query or create". But in a sense it's very limiting to accept only "nodeName.className" selectors, and users will want to extend it to more selectors (such as nodeName#id, nodeName.class1.class2, etc). Of course, it isn't possible to extend to arbitrary selectors (if we don't know the nodeName, we can't invent it, and supporting :nth-of-type or other subtle selectors is going to require a lot of magic tricks).

In the same vein, if this PR was accepted it would be consistent to support append("nodeName.className") and join("nodeName.className"), and maybe also with #id.

I often use the following pattern instead: g.html("").append('g').attr('class, 'x-axis'), and if this PR was approved, and append was logically extended, then we could do g.html("").append('g.x-axis') which is almost as straightforward as the proposal. (The only thing it doesn't do compared to one is to preserve whatever other nodes the original g contains.)

@curran
Copy link
Contributor Author

curran commented Mar 6, 2022

Thank you @Fil for giving this some thought.

I understand what you are saying regarding the conceptual problems it introduces.

it's very limiting to accept only "nodeName.className" selectors, and users will want to extend it to more selectors

I am of the opinion that it should be limiting, with its scope only supporting a single class. If folks want to use it and assign multiple classes, the existing API should be used.

In the same vein, if this PR was accepted it would be consistent to support append("nodeName.className") and join("nodeName.className"), and maybe also with #id.

That is interesting and I had not considered touching other parts of the API. But yes it might make sense. Assigning classes for the purpose of making specific D3 selections may well be one of the most common uses of .attr. So, making that easier might make a lot of sense. Such changes would be far reaching and would require an immense amount of consideration, though, which is well beyond the scope of my original idea. I personally have not encountered any code that uses #id for this purpose, but maybe it is done sometimes. I'm not sure there is a strong case for supporting #id here.

The only thing it (g.html("").append('g').attr('class, 'x-axis')) doesn't do compared to one is to preserve whatever other nodes the original g contains.
Indeed! Preserving existing nodes is an important feature for the use cases I've encountered (which are mainly around axes, guides and labels that each are uniquely classed <g> elements that are siblings and direct children of a common <svg> element).

To summarize the world in which this addition might make sense:

  • join("nodeName.className")
  • append("nodeName.className")

While those changes would compliment one conceptuallly, they do not block introduction of one and could be considered in the future.

@mbostock
Copy link
Member

mbostock commented Mar 6, 2022

I don’t want to introduce (and design and maintain) a new string-based DSL. D3 currently doesn’t need to understand how to parse a selector—it can just pass it through to element.querySelector or element.querySelectorAll. And likewise it can pass a tagName to document.createElement. If you want this sort of shorthand I suggest using d3-jetpack.

@curran
Copy link
Contributor Author

curran commented Mar 7, 2022

Got it. Thanks!

@curran curran closed this Mar 7, 2022
@curran curran mentioned this pull request Mar 7, 2022
@curran curran reopened this Mar 7, 2022
@curran
Copy link
Contributor Author

curran commented Mar 7, 2022

I have simplified the implementation to avoid a DSL:

const one = (selection, name, className) =>
  selection
    .selectAll(name + '.' + className)
    .data([null])
    .join(name)
    .attr('class', className);

@curran curran changed the title Introduce one One Mar 10, 2022
@curran curran marked this pull request as ready for review August 20, 2022 19:27
@curran
Copy link
Contributor Author

curran commented Aug 20, 2022

@Fil @mbostock I've taken a stab at documentation for this one, and also cleaned up the implementation. Ready for review. Thanks!

@IPWright83
Copy link

I'd most definitely use this, I think axis is the most common use case, but I've certainly a few more maybe specific ones where it'd be really handy.

@curran
Copy link
Contributor Author

curran commented Aug 22, 2022

Thanks! I find myself using this all the time for managing <g> elements that contain various things, like layers to control Z ordering, or containers for multiple visualizations (or legends) within the same <svg>. Also it's great for <text> labels like chart titles or axis labels. Also for singular visual things like <path> elements in line or area charts.

@curran
Copy link
Contributor Author

curran commented Aug 29, 2022

Most common use cases:

  • provision a top level SVG element
  • g elements to contain axes
  • g element with translate by margin left and top
  • g elements to contain layers (to control Z order)
  • g element for zooming
  • text labels
  • singular path elements for a line or area chart

src/one.js Outdated
@@ -0,0 +1,6 @@
export default (selection, name, className) =>
selection
.selectAll(`${name}.${className}`)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking at this again, className should probably be made optional. It's nice sometimes to omit a class name if it's not necessary.

@curran
Copy link
Contributor Author

curran commented Sep 25, 2022

Made some updates here:

  • make the class optional, which is useful when the class is not required. The class is only required in cases where we need to disambiguate between siblings of the same tag name, so no need to force the user to specify one unnecessarily.
  • update documentation
  • add tests

I've been using this utility in my work all the time and it works out great in that it reduces the amount of code significantly.

Is there anything else I should add here? Thanks!

@IPWright83
Copy link

Be great to get this in, I'd like to start using it immediately on some projects.

@curran
Copy link
Contributor Author

curran commented Oct 24, 2022

Example uses:

SVG Container

const svg = one(select(container), 'svg')
  .attr('width', width)
  .attr('height', height);

Axes

one(selection, 'g', 'x-axis')
  .attr('transform', `translate(0, ${height - bottom})`)
  .call(axisBottom(xScale));

one(selection, 'g', 'y-axis')
  .attr('transform', `translate(${left}, 0)`)
  .call(axisLeft(yScale));

Axis Labels

const g = one(selection, 'g', 'axis-labels')
  .attr('text-anchor', 'middle')
  .attr('font-size', '12px')
  .attr('font-family', 'sans-serif');

one(g, 'text', 'x-axis-label')
  .attr('x', left + (width - right - left) / 2)
  .attr('y', height - bottom + xLabelOffset)
  .attr('alignment-baseline', 'hanging')
  .text(xLabel);

one(g, 'text', 'y-axis-label')
  .attr('x', -(top + (height - bottom - top) / 2))
  .attr('y', left - yLabelOffset)
  .attr('transform', 'rotate(-90)')
  .text(yLabel);

Marks Container

one(selection, 'g', 'marks')
  .selectAll('circle')
  .data(data)
  .join('circle')
  .attr('cx', (d) => xScale(xValue(d)))
  .attr('cy', (d) => yScale(yValue(d)))
  .attr('r', circleRadius);

@curran
Copy link
Contributor Author

curran commented Jan 22, 2023

Still copying this implementation into fresh projects all the time! Anyone else using this?

@curran
Copy link
Contributor Author

curran commented Jan 22, 2023

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.

I can see the appeal, but I don't think this works. It feels both too specific and not strict enough.

It's usable only for ("element") or ("element", "class"); it cannot, for example, be used to add a unique element with a given aria-label, or id. And I don't think there is a way to make it more generic without taking care of the selector dsl, which we don't want to do anywhere in d3-selection.

It's not chainable (like .join is), so you have to write d3.one(d3.select(chart), "g", marks), not d3.select(chart).one("g", marks).

The implementation uses .attr("class", className), this will erase any class name that was already on the previous element (if there was one). In other words, updating that element will reset a class that we added to it. This might be desired, or not, depending on the application, but the API gives no clue that this is what is going to happen, and no way to change it in one direction or the other.

Instead, a user could do:

selection.selectAll("g").data([1]).join("g"); // element
selection.selectAll("g.marks").data([1]).join("g").attr("class", "marks");  // element, (re)sets class
selection.selectAll("g.marks").data([1]).join("g").classed("marks", true);  // element, adds className
selection.selectAll(`#${id}`).data([1]).join("g").attr("id", id);  // element, id

I can see how that is a bit tedious, because we specify things twice (element.className, element, className) instead of (element, className); but it's explicit about what it does, does not refer to a DSL, and is easier to adapt.

TBH I'm not a fan of .data(data).join("g") in the examples above. I often would want to use .join("g", data) instead—but it's for a separate discussion.

@curran
Copy link
Contributor Author

curran commented Jan 28, 2023

Thank you @Fil for your thoughtful reply!

Zooming out a bit, the core problem I'd like to see solved, somehow, is a concise way to write idempotent rendering logic that manages a single element. FWIW, this pattern is something I learned from studying the d3-axis implementation, which uses .data([null]) and a class.

The only reason I introduced a class is to handle the case where you want to manage a single element that's the same type as a sibling, and the class is just to disambiguate between them. You raise a great point that an id could also be used to do the same thing. You also raise a great point that this approach is not conducive to having multiple classes on the element because it clobbers any existing classes.

Regarging chaining, maybe that's a better API, the d3.select(chart).one("g", "marks") idea! But still, it suffers from all the other issues you raised.

I suppose I'm really just looking for the "best practice" of how to manage a single DOM element with D3. This may be the current best solution:

selection.selectAll("g.marks").data([1]).join("g").attr("class", "marks");

It's just disappointing to me how verbose this is, because I find myself writing this same logic over and over again in projects and it really adds up. For the use cases I've encountered, I have not ever actually needed to put multiple classes on elements managed in this way, so the class clobbering issue is not something I've faced in practice.

Anyway, thanks for looking at it, I appreciate the effort! It looks like this PR will never get merged because the pattern is fundamentally flawed. I'd like to keep searching for the right solution though, the "best practice" for managing a singular element with D3.

MOHAMMADALI1490740

This comment was marked as spam.

@curran
Copy link
Contributor Author

curran commented Feb 9, 2023

Here's another example of code that could be made less verbose:

import { axisLeft, axisBottom } from 'd3';

export const axes = (
  selection,
  { xScale, yScale }
) => {
  selection
    .selectAll('g.y-axis')
    .data([null])
    .join('g')
    .attr('class', 'y-axis')
    .attr(
      'transform',
      `translate(${xScale.range()[0]},0)`
    )
    .call(axisLeft(yScale));

  selection
    .selectAll('g.x-axis')
    .data([null])
    .join('g')
    .attr('class', 'x-axis')
    .attr(
      'transform',
      `translate(0,${yScale.range()[0]})`
    )
    .call(axisBottom(xScale));
};

Any suggestions for making this more concise using the existing D3 APIs?

@Fil
Copy link
Member

Fil commented Feb 9, 2023

I called ChatGPT for help refactoring:

const createAxis = (sel, sc, ax, t) => sel
  .selectAll(`g.${ax}`).data([null]).join('g')
  .attr('class', ax).attr('transform', `translate(${t})`)
  .call(ax(sc));
export const axes = (sel, { x, y }) => {
  createAxis(sel, y, axisLeft, [x.range()[0], 0]);
  createAxis(sel, x, axisBottom, [0, y.range()[0]]);
};

Note that ChatGPT is slightly wrong, but you get the idea. I think we can stop here, maybe continue the discussion on slack instead.

@Fil Fil closed this Feb 9, 2023
@curran
Copy link
Contributor Author

curran commented Feb 11, 2023

Indeed. Thanks!

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.one
5 participants