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

use x, y and dy when there is no rotate #682

Merged
merged 3 commits into from
Jan 19, 2022
Merged

Conversation

Fil
Copy link
Contributor

@Fil Fil commented Jan 19, 2022

test multiline (with 1 empty line)
document

test multiline (with 1 empty line)
document
@Fil Fil requested a review from mbostock January 19, 2022 14:33
Copy link
Member

@mbostock mbostock left a comment

Choose a reason for hiding this comment

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

Doesn’t this break the layout of multiline text, since the tspans assume that x and y are relative to the transform, which is no longer present? That’s why I adopted the transform in the first place.

And perhaps more importantly, is there a reason to do this? I was trying to decide the answer to this question. I don’t know if there’s a compelling reason to do this (such as performance) and hoped you’d help me think that through.

Also I am reminded that the changelog will need to mention the change in behavior for dx and dy. If I’m not mistaken, in addition to the loss of units, they now apply before the rotation rather than after. This makes me wonder if we want something like the old dx and dy in addition to the new one?

@Fil
Copy link
Contributor Author

Fil commented Jan 19, 2022

In my test notebook https://observablehq.com/@fil/test-text-multiline-677 it seems to work, but maybe there's a case I didn't see? (I don't have a strong opinion on transform vs x/y, I must say.)

[THE CHOIR: YES, THERE WAS SOMETHING HE DIDN'T SEE]

@mbostock
Copy link
Member

Okay, how strange. I must have gotten confused in my implementation. Thanks!

@mbostock mbostock merged commit 0b58a98 into mbostock/multiline Jan 19, 2022
@mbostock mbostock deleted the fil/multiline branch January 19, 2022 16:09
mbostock added a commit that referenced this pull request Jan 19, 2022
* multiline text

* lineHeight option

* skip empty lines

* use x, y and dy when there is no rotate (#682)

* use x, y and dy when there is no rotate
test multiline (with 1 empty line)
document

* use selection.call to branch

* remove duplicate test

Co-authored-by: Mike Bostock <[email protected]>

* minimize diff

* fix choropleth label alignment

* more test fixes

* tweak wheel example

* Update README

* Update README

* Update README

Co-authored-by: Philippe Rivière <[email protected]>
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.

2 participants