Uh oh!
There was an error while loading. Please reload this page.
Fix/legend resize - #356
Conversation
timelyportfolio
commented
Mar 24, 2016
mistake 1 forgot to lint. doing now. |
fd2351c to
94bd5ffCompare| 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!
mdtusz
commented
Mar 24, 2016
This looks pretty good to me. We'll wait for the all-clear from @etpinard then merge it in! |
etpinard
commented
Mar 25, 2016
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 |
etpinard
commented
Mar 28, 2016
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? |
timelyportfolio
commented
Mar 29, 2016
@etpinard I should be able to wrap this up today. |
etpinard
commented
Mar 29, 2016
Great. @timelyportfolio can you also check that this example works as expected? Thanks! |
timelyportfolio
commented
Mar 29, 2016
@etpinard yes, will be happy to check this also |
c012d74 to
43d430cCompare| // 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.
a565dc6 to
3e6b949Comparetimelyportfolio
commented
Mar 30, 2016
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. |
etpinard
commented
Mar 30, 2016
@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 🍻 |
timelyportfolio
commented
Mar 30, 2016
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.