-
Notifications
You must be signed in to change notification settings - Fork 176
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
frameAnchor; fix multiline text #686
Conversation
src/style.js
Outdated
/-?left$/.test(frameAnchor) ? marginLeft : /-?right$/.test(frameAnchor) ? width - marginRight : (marginLeft + width - marginRight) / 2, | ||
/^top-?/.test(frameAnchor) ? marginTop : /^bottom-?/.test(frameAnchor) ? height - marginBottom : (marginTop + height - marginBottom) / 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need the -?
here since the keywords have already been validated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do, because you can have e.g. “top” or “top-left” and we want to match on both.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see what you mean, because /^top/
will match both, and nothing else. That’s true, but I was a little worried that we’d introduce another name in the future that would coincidentally match the same prefix. I think it’s clearer to have the hyphen as an explicit delimiter even though it’s not strictly necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant that matching "top" or "left" seems to be enough. Just a nit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I thought about /^top($|-)/
instead, but we’d have to change this code if we introduce new frameAnchor values anyway, so it feels fine to take your suggestion.
I'm confused by something—writing it quickly since we're having dinner. I'm trying to add a multiline explainer on top of the aapl-bollinger example. Ideally I should be able to say
But this fails in two ways:
For the point 1, I suppose we could have frameAnchor setting the default x and y. For point 2 it's probably due to something I didn't see in #682. bbl |
For the first, that’s expected because Plot.text defaults x and y to first and second, respectively, using maybeTuple. Line 105 in b5a1724
Lines 96 to 99 in b5a1724
If you want to change that, we’d need to do something like type inference on the data, and default x and y to null if it’s an array of strings. Or perhaps on the presence of the frameAnchor option? Maybe that’s useful for text annotations? 🤷 For the second, that’s the brokenness I was expecting here #682 (review). With #682, you get something like this: <text x="639.5" y="0.5">
<tspan x="0" y="-0.18em">Multiline</tspan>
<tspan x="0" y="0.8200000000000001em">explainer</tspan>
</text> But the x and y on the tspans aren’t additive with those on the text: the x and y on the text are now ignored. So we have to use a transform attribute when we generate multiline text. I’ll revert this part. |
This seems to work pretty well: Plot.plot({
marks: [
Plot.frame(),
Plot.text([`Multiline\nexplainer`], {
textAnchor: "end",
lineAnchor: "top",
frameAnchor: "top-right",
text: d => d
})
]
})
|
Okay, changed it so that if you specify a frameAnchor option, x and y now default to null instead of maybeTuple. That makes sense to me, since you’d never use the two of them together. |
Fixes #683.