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

href channel #645

Merged
merged 4 commits into from
Jan 2, 2022
Merged

href channel #645

merged 4 commits into from
Jan 2, 2022

Conversation

mbostock
Copy link
Member

@mbostock mbostock commented Jan 1, 2022

Adds a generic href channel that wraps the specified mark elements with an A element with the appropriate xlink:href attribute. A target option is also available (e.g., “_blank”). I didn’t implement it for grouped marks (lines and areas), but maybe I should. Fixes #283.

@mbostock mbostock requested a review from Fil January 1, 2022 22:11
src/style.js Outdated
Comment on lines 153 to 156
const a = document.createElementNS(namespaces.svg, "a");
a.setAttributeNS(namespaces.xlink, "href", href[i]);
if (target != null) a.setAttribute("target", target);
this.parentNode.insertBefore(a, this).appendChild(this);
Copy link
Contributor

Choose a reason for hiding this comment

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

makes we wish we could do selection.wrap("svg:a") (related: d3/d3-selection#296)

Copy link
Contributor

Choose a reason for hiding this comment

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

selection.wrap: d3/d3-selection#297

@Fil
Copy link
Contributor

Fil commented Jan 2, 2022

wonder if we shouldn't have target: "_blank" by default?

@mbostock
Copy link
Member Author

mbostock commented Jan 2, 2022

wonder if we shouldn't have target: "_blank" by default?

I don’t think so, no. I do wonder whether it’s possible to set the equivalent of a <base target> for the SVG though, so we don’t have to repeat the target option everywhere? It’d be nice to set it on the base plot somehow… but it’s not a huge deal to set it on each link, too.

@mbostock
Copy link
Member Author

mbostock commented Jan 2, 2022

Any other changes, @Fil? Or LGTY?

Copy link
Contributor

@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.

it's good, we'll just have to add tests

* cheeky test plot for href

* tweak test; add snapshot

Co-authored-by: Mike Bostock <[email protected]>
@mbostock mbostock merged commit 9c84cdd into main Jan 2, 2022
@mbostock mbostock deleted the mbostock/href branch January 2, 2022 20:23
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.

Hyperlinks?
2 participants