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

fix clip: frame for band scales #1204

Merged
merged 5 commits into from
Jan 2, 2023
Merged

fix clip: frame for band scales #1204

merged 5 commits into from
Jan 2, 2023

Conversation

mbostock
Copy link
Member

Fixes #1202. The previous fix in #950 was only partly correct: in essence, it was trying to undo transform applied by applyTransform:

plot/src/style.js

Lines 383 to 389 in c095660

export function applyTransform(selection, mark, {x, y}, tx = offset, ty = offset) {
tx += mark.dx;
ty += mark.dy;
if (x?.bandwidth) tx += x.bandwidth() / 2;
if (y?.bandwidth) ty += y.bandwidth() / 2;
if (tx || ty) selection.attr("transform", `translate(${tx},${ty})`);
}

But the behavior of applyTransform varies from mark to mark, and even within a mark depending on which channels are applied, for example for collapsed rects:

.call(applyTransform, this, {x: X1 && X2 && x, y: Y1 && Y2 && y}, 0, 0)

Rather than trying to undo the transform, this fix instead wraps the G element with another (untransformed) G element, applying the clip to the parent G element. And thus the clipped region is not affected by the mark’s transform.

To simplify the adoption of this fix, applyIndirectStyles now mutates the passed-in selection.node to return the parent G element when clipping is applied. This means that callers effectively get the fix “for free” (and we now don’t even need to pass scales to applyIndirectStyles). I’m sure there’s a cleaner way to do this, but this seems good for now and minimizes churn.

Previously #949.

@mbostock mbostock requested a review from Fil December 30, 2022 20:35
@mbostock mbostock added the bug Something isn’t working label Dec 30, 2022
test/plots/band-clip.js Outdated Show resolved Hide resolved
mbostock and others added 2 commits January 2, 2023 09:20
Co-authored-by: Philippe Rivière <fil@rezo.net>
@mbostock mbostock merged commit b0434aa into main Jan 2, 2023
@mbostock mbostock deleted the mbostock/fix-clip branch January 2, 2023 17:38
chaichontat pushed a commit to chaichontat/plot that referenced this pull request Jan 14, 2024
* fix clip: frame for band scales

* Update test/plots/band-clip.js

Co-authored-by: Philippe Rivière <fil@rezo.net>

* comment

* lift aria attributes

* comment

Co-authored-by: Philippe Rivière <fil@rezo.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn’t working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The clip option is misaligned with band scales
2 participants