Fix/legend resize #356
Fix/legend resize #356
Conversation
|
mistake 1 forgot to lint. doing now. |
timelyportfolio
force-pushed
the
fix/legend-resize
branch
from
fd2351c to
94bd5ff
Compare
|
|
||
| var scrollBarTrack = scrollheight - constants.scrollBarHeight - 2 * constants.scrollBarMargin, | ||
| translateY = scrollBox.attr('data-scroll'), | ||
| scrollBoxY = Lib.constrain(translateY - delta, Math.min(scrollheight - opts.height, 0), 0), | ||
| scrollBoxY = Lib.constrain(translateY - delta, scrollheight-opts.height, 0), |
There was a problem hiding this comment.
🐄 spacing between scrollheight and opts.height.
There was a problem hiding this comment.
oh thanks. should have thought about using d3 wheel also. great change!
|
This looks pretty good to me. We'll wait for the all-clear from @etpinard then merge it in! |
|
Yep. I like this. I was worried about using d3 events looking forward to d3's 4.0 release. But looks like events on selection will be part of the stock d3-selection package, which we'll obviously need. So 🍻 great solution! @timelyportfolio Can you add a test case in |
|
It would be nice to merge this one in for the 1.7.1 release at some point this week. @timelyportfolio, are you going to have time to work on adding a test case in the next few days? |
|
@etpinard I should be able to wrap this up today. |
|
Great. @timelyportfolio can you also check that this example works as expected? Thanks! |
|
@etpinard yes, will be happy to check this also |
timelyportfolio
force-pushed
the
fix/legend-resize
branch
3 times, most recently
from
c012d74 to
43d430c
Compare
|
|
||
| // clippath resized to new height less than new plot height | ||
| expect(+legendHeight).toBe(getPlotHeight(gd)); | ||
| expect(+legendHeight).toBeLessThan(+origLegendHeight); |
There was a problem hiding this comment.
Could you check against the computed height using toBeCloseTo?
It helps catch potential bugs earlier than just relative size comparison.
There was a problem hiding this comment.
sure, will be happy to; sorry for the flurry of commits; my local set up broke this morning.
timelyportfolio
force-pushed
the
fix/legend-resize
branch
7 times, most recently
from
a565dc6 to
3e6b949
Compare
|
I'm having a very difficult time debugging these simple tests with testing broken on my devel machine. I don't want to hold up the 1.7.1 release. Please let me know if you can help me see the obvious that I am missing. Thanks. Will try again tomorrow. |
|
@timelyportfolio I pulled down your branch on added a test fixup commit in #365. This issue is now a week old, and quite a few users have noticed its effects, so I decided to get your patch in ASAP. Thanks for work 🍻 |
|
thanks! |
This is my first attempt at addressing #348 and plotly/plotly.R#525. In summary, I
if(firstRender==trueblock as suggested in @etpinard issue commentclipPathenter,append,exit pattern to update on resizescrollheightargument to thescrollHandlerand removeeventListenerson resize to avoid scope problemsd3.behavior.drag()which should properly handle touch and allow to work properly in RStudio viewerSince this is my first pull, I apologize if I missed any of the contribution guidelines. Also, other than a visual test, I was not sure how to test. I would appreciate some advice here.