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

Treemap text positions for out of range cases when transitioning with maxdepth #4227

Merged
merged 5 commits into from
Sep 30, 2019

Conversation

archmoj
Copy link
Contributor

@archmoj archmoj commented Sep 26, 2019

Fixes the jump happening at the end of entering new text elements of treemap graph when maxdepth is set.

In the example below the inner text grows too big during the transition and then returns to its normal on transition end.

Peek 2019-09-25 11-08

Codepen before
Codepen after

@etpinard

@archmoj archmoj added bug something broken status: reviewable labels Sep 26, 2019
@archmoj archmoj added this to the v1.50.0 milestone Sep 26, 2019
@@ -1225,7 +1225,7 @@ describe('Test treemap tweening:', function() {
'M284.375,188.5L548.375,188.5L548.375,308.5L284.375,308.5Z'
);
_assert('move B text to new position', 'transform', 'B', [220.25126, 0]);
_assert('enter b text to new position', 'transform', 'b', [287.375195, 5]);
_assert('enter b text to new position', 'transform', 'b', [284.66071, 35714285714286]);
Copy link
Contributor

@etpinard etpinard Sep 26, 2019

Choose a reason for hiding this comment

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

That 35714285714286 y-translate coordinate looks odd. Is this node support to go way off the screen?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@etpinard No that should stay on the screen.
I have no idea why we need that only for the test to pass on the CI.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have no idea why we need that only for the test to pass on the CI.

Can you try to find out why?

Copy link
Contributor

@etpinard etpinard Sep 26, 2019

Choose a reason for hiding this comment

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

... e.g. by first comparing this 35714285714286 value with the before and after transition values

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK. Let me find out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is related to using one space character instead of a blank string.
With that the test is OK.

Copy link
Contributor

Choose a reason for hiding this comment

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

So, what y-translate value do we get if we use a blank string?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

[287.375195, 0] if we use blank. But then the transition doesn't look good.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok - well this is hardly a satisfactory answer, but hey the transition looks better now, so I'll stop complaining.

@etpinard
Copy link
Contributor

https://codepen.io/MojtabaSamimi/pen/GRKLNLe?editors=0010 looks amazing @archmoj - thanks very much!

- cast _text to pt for easier debugging
- pass pt object to the function instead of separated arguments
- do not return in the middle of draw functions
- expand void area for blank text
@@ -1127,7 +1127,7 @@ describe('Test treemap tweening:', function() {
if(attrName === 'transform') {
var fake = {attr: function() { return actual; }};
var xy = Drawing.getTranslate(fake);
expect([xy.x, xy.y]).toBeWithinArray(exp, 1, msg2);
expect([xy.x, xy.y]).toBeWithinArray(exp, 2, msg2);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Increase range so that the test pass on my machine.

@archmoj
Copy link
Contributor Author

archmoj commented Sep 27, 2019

The image test would pass if we cherry pick 422d5d3.

@archmoj
Copy link
Contributor Author

archmoj commented Sep 28, 2019

After spending good amount of time on testing with different devices, the transition appears to work smoothly after 6d6f946.
Now setting the PR status to reviewable.

@etpinard
Copy link
Contributor

etpinard commented Sep 30, 2019

After spending good amount of time on testing with different devices, the transition appears to work smoothly after 6d6f946.

Great. In which scenario did transitions look "bad" before that latest commit 6d6f946 ?

@archmoj
Copy link
Contributor Author

archmoj commented Sep 30, 2019

After spending good amount of time on testing with different devices, the transition appears to work smoothly after 6d6f946.

Great. In which scenario did transitions look "bad" before that latest commit 6d6f946 ?

Setting undefined positions to zeros (made in a1a2a2b in this PR) was not a good idea. That could result in all the text for new points enter from the top left corner of the viewport which was not desirable. Because of that the proper condition checks are added back in 6d6f946.

@etpinard
Copy link
Contributor

Setting undefined positions to zeros (made in a1a2a2b in this PR) was not a good idea. That could result in all the text for new points enter from the top left corner of the viewport which was not desirable. Because of that the proper condition checks are added back in 6d6f946.

Got it. Thanks!!


This PR is good to merge 💃

Feel free to cherry-pick 422d5d3 onto this branch and merge it - or wait until #4219 is merged to merge master into this branch.

@archmoj
Copy link
Contributor Author

archmoj commented Sep 30, 2019

@etpinard
Just want to know which way you prefer:
Now that there is no merge conflict with master, is it OK if I merge this to master without merging master into this?

@etpinard
Copy link
Contributor

Now that #4219 is merged, please merge the current master into this branch

@archmoj archmoj merged commit 4420593 into master Sep 30, 2019
@archmoj archmoj deleted the treemap-text-transitions-maxdepth branch September 30, 2019 18:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something broken
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants