-
Notifications
You must be signed in to change notification settings - Fork 7.6k
Conversation
…ar-resize-refactor
@@ -62,11 +62,11 @@ html, body { | |||
body { | |||
.vbox; | |||
|
|||
&.resizing a, &.resizing #projects a, &.resizing .main-view, &.resizing .CodeMirror-lines { | |||
&.horz-resizing a, &.horz-resizing #projects a, &.horz-resizing .main-view, &.horz-resizing .CodeMirror-lines, &.horz-resizing .CodeMirror-gutter-text { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be much better if we didn't need to update this rule every time someone adds a new panel. This should only be the "resizing" and "horz-resizing" classes. Maybe we need to adjust where those classes are applied, but I think it's worth the change.
Same comment for next rule with "vert-resizing".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll look into it before creating the pull request again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@redmunds It's okay if we do $("*").toggleClass("vert-resizing")
on the Resizer
module? Or could that create some hit on performance?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That will remove the "vert-resizing" class from the few elements that have it, and add it to every other element in the DOM! I don't think that's what you want to do.
You can use $(".vert-resizing").toggleClass("vert-resizing") to remove the "vert-resizing" class from all elements that have it applied. Note: after that you won't be able to identify the elements that used to have it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assuming that's what you want, this is a little better: $(".vert-resizing").removeClass("vert-resizing")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes... I was actually thinking of doing $("*").addClass("vert-resizing")
on resize start and then the removeClass on the resize end. This way, the handle cursor isn't overridden by other rules, and we can take the resizing classes outside and shouldn't be affected by any element changing the cursor style...
I assume that's what you meant in the beginning.. or maybe I'm missing the point?
If this is not entirely satisfactory, maybe we can leave it for now and think it through.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What you're saying makes sense, but won't $("*") return every element in the entire Brackets DOM tree? I think you want to limit it to the panel that's being resized. We can talk about it more when you push up a pull request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's why I wanted to avoid it. I expect $("*") to be really slow.
I think I've found a nice solution that could work creating a transparent div just for the resizing operation. I'll put together a pull request and then we can discuss it there.
Thanks for the support!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It sounds like there are 3 independent changes here:
Is that correct? If so, it wold be much nicer if you could separate them into different pull requests. It's always best to make pull requests as atomic as possible. |
Of course, sorry about that! I'm closing this one and will be opening separate pull requests for the different changes. |
Hi,
Following the inclussion of
utils/Resizer
in #1661, I've refactoredproject/SideBarView
to use the existing code.I've added a new
toggleVisibility
API inResizer
. This can be called for any resizable panel. Panels with a classcollapsable
are also toggled when double clicking on their resize handler. Resizable panels also dispatch nowpanelCollapsed
andpanelExpanded
when changing from one state into another.This pull request includes also some fixes for the current sidebar resize behavior (which could be merged independently if this whole refactorization is not really required):