Resize events consolidation and minor resizer bugs (#2079) #2092
Conversation
| @@ -233,6 +244,7 @@ define(function (require, exports, module) { | |||
| startPosition = e[directionProperty], | |||
| startSize = $element.is(":visible") ? elementSizeFunction.apply($element) : 0, | |||
| newSize = startSize, | |||
| previousSize = startSize, | |||
There was a problem hiding this comment.
Now that I think of it, currentSize may be a better name for this...
There was a problem hiding this comment.
I think previousSize is an ok enough name.
ghost
assigned 
|
Just tested the code a bit and the stream of events looks much cleaner now! I'll review the code itself now. |
| } | ||
|
|
||
| if (elementPrefs.contentSize !== undefined) { | ||
| contentSizeFunction.apply($resizableElement, [Math.max(elementPrefs.contentSize, minSize)]); | ||
| contentSizeFunction.apply($resizableElement, [elementPrefs.contentSize]); |
There was a problem hiding this comment.
Any reason you removed the checks for minSize?
There was a problem hiding this comment.
Oops, yeah... I forgot to comment on that. The previous logic was wrong and somehow misleading and I wanted to check with you the best approach to take here.
Now that the panels have min-size, the only way for minSize to be chosen here is if we change the minimum size for the panel to a bigger value than the one stored in the preferences in the previous session. With that in mind, we can do two things:
- Don't check for minSize. This shows the panel with the stored sizes and the moment you start resizing it jumps to the new minimum size directly.
- Check for minSize (like previously), which should automatically resize the panel to the minSize.
There were a couple of issues with 2 before:
- If you resize the panel to the minimum size (in which case
size==minSize),contentSizeis always smaller thanminSizeand we are forcing always a bigger size on it (the scrollbars aren't quite right). - If you raise minSize, the panel is resized correctly, but again the content isn't and it is always getting resized to minSize. We'd probably need to set it to
minSize-(size-contentSize). - We may want to save the preferences in this case, although the code won't complain.
Which behavior do you prefer? If it's the second, I could push it along with the rest of review changes or fix it in a separate issue (I'd say that without the checks, the code at least behaves is it says it does)
|
Done reviewing. Looks pretty solid overall -- thanks for posting the cleanup so fast! |
|
@peterflynn Changes pushed. Ready for re-review. Includes fix for #2117 as discussed (pending the mouse-glitch). Doesn't fix the check for minSize in the preferences, which could be addressed in a separate issue. |
|
@jbalsas: Sorry I took so long to get back to you! This looks solid to me. Re the min-size issue: your explanation makes sense to me. I don't think we should worry right now about the edge case of min-size increasing after a smaller value is already stored in prefs. Re requesting animation frames in mousemove vs. at the end of doRedraw(): your latest code still requests at the end of each doRedraw() -- was that your intent? I think it's fine, myself... there's virtually no overhead from extra frames between mousemoves because of the newSize check. |
|
Don't worry! About requesting animation frames, I was just testing different approaches to see if if it could still be improved a little. As you say, with the newSize check, the impact of doRedraw() is minimal, so I decided to leave it as is. Feel free to merge this anytime ;) |
|
Sounds good to me -- merging |
This pull request consolidates the resize events dispatched by
utils/Resizerfollowing the comments on (#2079).It also fixes some minor resizer bugs such as the one described in #2079 where after dragging the panel shut, it will expand to 10px instead of the original size.
@peterflynn Let me know if this is closer to what you'd expect