Fix for issue #1122 #1661

Merged
merged 17 commits into from Oct 8, 2012

Conversation

Projects
None yet
2 participants
@jbalsas
Contributor

jbalsas commented Sep 16, 2012

This pull request provides a possible fix for issue #1122.

It adds resize functionality for the two existing bottom panels ("jslint errors" and "find in files").

Knwon issues

  • If a panel is resized beyond the main toolbar, it keeps growing. If the mouse is released within the toolbar the panel height is greater than the main view height. When starting the panel resize again, there is an offset between the handler and the panel height due to this.
  • If both panels are opened, resizing the search panel until both the panels cover the entire editor and keep going causes the same effect as the previous issue.
  • If both panels are opened, resizing the jslint panel beyond the main toolbar causes the find panel to shift downwards as the first one keeps growing.

This issues could be solved but it will require to introduce some dependencies between panels or dom elements and betwen panels and the toolbar element.

@ghost ghost assigned redmunds Sep 17, 2012

@redmunds

This comment has been minimized.

Show comment Hide comment
@redmunds

redmunds Sep 17, 2012

Contributor

Thanks for submitting this code. We have an internal milestone that we're trying to hit this week, so I might not get to reviewing this code until next week.

Contributor

redmunds commented Sep 17, 2012

Thanks for submitting this code. We have an internal milestone that we're trying to hit this week, so I might not get to reviewing this code until next week.

@redmunds

This comment has been minimized.

Show comment Hide comment
@redmunds

redmunds Sep 26, 2012

Contributor

This is cool, but I'm seeing a couple problems with this on Win7. I posted a pic here: https://twitter.com/randyedmunds/status/250976078926077952/photo/1 (Let me know if the resolution got messed up and I can e-mail the pic).

Notice in the upper (JSLint) panel that after increasing the height of the panel, I still see the same number of lines of JSLint errors displayed (with a vertical scrollbar). The number of lines viewed should be increased to fill the height of the panel.

Notice the lower (Find in Files) panel that the number of lines are increased with the height, but on the lower end of the scrollbar, the down-arrow button is mostly off the screen.

Contributor

redmunds commented Sep 26, 2012

This is cool, but I'm seeing a couple problems with this on Win7. I posted a pic here: https://twitter.com/randyedmunds/status/250976078926077952/photo/1 (Let me know if the resolution got messed up and I can e-mail the pic).

Notice in the upper (JSLint) panel that after increasing the height of the panel, I still see the same number of lines of JSLint errors displayed (with a vertical scrollbar). The number of lines viewed should be increased to fill the height of the panel.

Notice the lower (Find in Files) panel that the number of lines are increased with the height, but on the lower end of the scrollbar, the down-arrow button is mostly off the screen.

Use outerHeight() for toolbars height to account for paddings and mar…
…gins

Fixed missing toolbar.outerHeight() in _setHeight for jsLint
@jbalsas

This comment has been minimized.

Show comment Hide comment
@jbalsas

jbalsas Sep 26, 2012

Contributor

Hi!

I think I messed up the jslint results panel resize when doing some cleaning before the pull request... fixed now :)

About the other issue, scrollbars on Mac are somehow subtler so this was harder to spot than on windows. I was using height() to get the toolbars height, which wasn't considering paddings and margins. I've changed it to outerHeight() instead. Please, check it to see if is ok now.

Contributor

jbalsas commented Sep 26, 2012

Hi!

I think I messed up the jslint results panel resize when doing some cleaning before the pull request... fixed now :)

About the other issue, scrollbars on Mac are somehow subtler so this was harder to spot than on windows. I was using height() to get the toolbars height, which wasn't considering paddings and margins. I've changed it to outerHeight() instead. Please, check it to see if is ok now.

@redmunds

This comment has been minimized.

Show comment Hide comment
@redmunds

redmunds Sep 27, 2012

Contributor

Excellent. Both of the problems mentioned above are fixed. I am noticing another problem with releasing mouse capture. Do this:

  1. Open JSLint panel (it only happens with JSLint panel, either by itself or with FiFs panel)
  2. Drag height up as high as you can (until editor disappears)
  3. Release left mouse button
  4. Now move mouse down over JSLint panel

Results: JSLint panel height starts decreasing as if mouse button was never released, so it seems to still have the capture.

Expected: Obviously, releasing the mouse should stop the drag, but I'm not sure if I like how the entire editor can be covered up. I am wondering if enforcing a minimum height on the editor will fix both problems?

Contributor

redmunds commented Sep 27, 2012

Excellent. Both of the problems mentioned above are fixed. I am noticing another problem with releasing mouse capture. Do this:

  1. Open JSLint panel (it only happens with JSLint panel, either by itself or with FiFs panel)
  2. Drag height up as high as you can (until editor disappears)
  3. Release left mouse button
  4. Now move mouse down over JSLint panel

Results: JSLint panel height starts decreasing as if mouse button was never released, so it seems to still have the capture.

Expected: Obviously, releasing the mouse should stop the drag, but I'm not sure if I like how the entire editor can be covered up. I am wondering if enforcing a minimum height on the editor will fix both problems?

@redmunds

View changes

src/styles/brackets_variables.less
@@ -50,5 +50,6 @@
@z-index-brackets-sidebar-resizer: @z-index-brackets-ui + 2;
@z-index-brackets-resizer-div: @z-index-brackets-sidebar-resizer + 1;
+@z-index-brackets-bottom-resizer: @z-index-brackets-ui + 2;

This comment has been minimized.

Show comment Hide comment
@redmunds

redmunds Sep 27, 2012

Contributor

@z-index-brackets-bottm-panel-resizer would be more accurate.

@redmunds

redmunds Sep 27, 2012

Contributor

@z-index-brackets-bottm-panel-resizer would be more accurate.

This comment has been minimized.

Show comment Hide comment
@jbalsas

jbalsas Sep 27, 2012

Contributor

will fix...

@jbalsas

jbalsas Sep 27, 2012

Contributor

will fix...

@redmunds

View changes

src/language/JSLintUtils.js
+ * @private
+ * Sets jslint panel height and resizes editor.
+ * @param {number} height Height in pixels.
+ */

This comment has been minimized.

Show comment Hide comment
@redmunds

redmunds Sep 27, 2012

Contributor

There's a lot of duplicated code here that needs to be refactored into a single resizer class. There are several other extensions (and I expect more to come) that also use the bottom panel adn would like to have this same functionality:

@redmunds

redmunds Sep 27, 2012

Contributor

There's a lot of duplicated code here that needs to be refactored into a single resizer class. There are several other extensions (and I expect more to come) that also use the bottom panel adn would like to have this same functionality:

This comment has been minimized.

Show comment Hide comment
@jbalsas

jbalsas Sep 27, 2012

Contributor

Didn't thought about the other extensions... should we refactor this in a utils/Resizer module with a simple API to make an element or extension resizable and initialize its behavior?

@jbalsas

jbalsas Sep 27, 2012

Contributor

Didn't thought about the other extensions... should we refactor this in a utils/Resizer module with a simple API to make an element or extension resizable and initialize its behavior?

This comment has been minimized.

Show comment Hide comment
@redmunds

redmunds Sep 27, 2012

Contributor

Yes.

@redmunds

redmunds Sep 27, 2012

Contributor

Yes.

@redmunds

View changes

src/htmlContent/main-view.html
@@ -95,12 +95,15 @@
</div>
<div id="jslint-results" class="bottom-panel">
+ <div id="jslint-resizer" class="h-resizer"></div>

This comment has been minimized.

Show comment Hide comment
@redmunds

redmunds Sep 27, 2012

Contributor

Would be nice if the resizing functionality could be added to a bottom panel simply by adding a the h-resizer class to the parent container and this DIV was dynamically added to the DOM. This would make it easier and less obtrusive to add.

@redmunds

redmunds Sep 27, 2012

Contributor

Would be nice if the resizing functionality could be added to a bottom panel simply by adding a the h-resizer class to the parent container and this DIV was dynamically added to the DOM. This would make it easier and less obtrusive to add.

This comment has been minimized.

Show comment Hide comment
@jbalsas

jbalsas Sep 27, 2012

Contributor

Would it be okay to have a module like utils/Resizer and scan for resizer classes on initialization to add the necessary elements as well as configure the behavior?

It could work also on the sidebar...

@jbalsas

jbalsas Sep 27, 2012

Contributor

Would it be okay to have a module like utils/Resizer and scan for resizer classes on initialization to add the necessary elements as well as configure the behavior?

It could work also on the sidebar...

This comment has been minimized.

Show comment Hide comment
@redmunds

redmunds Sep 27, 2012

Contributor

Yes, that sounds like the best solution.

@redmunds

redmunds Sep 27, 2012

Contributor

Yes, that sounds like the best solution.

@jbalsas

This comment has been minimized.

Show comment Hide comment
@jbalsas

jbalsas Sep 27, 2012

Contributor

I basically thought of this as a temporary fix while getting a crack at the tabbed panel solution discussed in https://groups.google.com/forum/?fromgroups=#!topic/brackets-dev/PqEbuhw7k6c. I feel, that would be the best way to go.

We should of course improve this if it is to be made part of Brackets in the meantime.

About the behavior you saw, I think it only happens when you release the mouse outside the Brackets window (also over the app toolbar). It also happens for the FiFs panel. I'll try to fix this later today.

About enforcing a minimum height for the editor, I did try it, but it didn't convince me at all. There are lots of issues to be addressed for that:

1. Dependencies between panels

We'd need to check the height of all visible panels to get the available space. I didn't want to create any dependencies between panels. Having a utils/Resizer class that tracks all panels could help with this...

2. Resizing the window

  • With one panel

If we start resizing the whole window... what should happen when the minimum height for the editor is reached? Should the panel shrink to leave place for the editor?

  • With two or more panels

If we start resizing the whole window, when reaching the minimum height for the editor, should the panels shrink? Should all panels shrink in the same proportion? Ones more than others?

3. Resizing the panels

If we start resizing a panel and the maximum height for all panels is reached, should we stop all resizing or should other panels give height away for or it?

4. Opening panels

If we try to open a panel and there is not enough space for the saved height of the panel, should we set it to the maximum available height? What if no space at all is left?

I think solving all these issues may be a lot of work that could be better spent on the tabbed panel solution if that is the final goal. I'm willing to work in both directions in any case, so just point at what should we do, please :)

Contributor

jbalsas commented Sep 27, 2012

I basically thought of this as a temporary fix while getting a crack at the tabbed panel solution discussed in https://groups.google.com/forum/?fromgroups=#!topic/brackets-dev/PqEbuhw7k6c. I feel, that would be the best way to go.

We should of course improve this if it is to be made part of Brackets in the meantime.

About the behavior you saw, I think it only happens when you release the mouse outside the Brackets window (also over the app toolbar). It also happens for the FiFs panel. I'll try to fix this later today.

About enforcing a minimum height for the editor, I did try it, but it didn't convince me at all. There are lots of issues to be addressed for that:

1. Dependencies between panels

We'd need to check the height of all visible panels to get the available space. I didn't want to create any dependencies between panels. Having a utils/Resizer class that tracks all panels could help with this...

2. Resizing the window

  • With one panel

If we start resizing the whole window... what should happen when the minimum height for the editor is reached? Should the panel shrink to leave place for the editor?

  • With two or more panels

If we start resizing the whole window, when reaching the minimum height for the editor, should the panels shrink? Should all panels shrink in the same proportion? Ones more than others?

3. Resizing the panels

If we start resizing a panel and the maximum height for all panels is reached, should we stop all resizing or should other panels give height away for or it?

4. Opening panels

If we try to open a panel and there is not enough space for the saved height of the panel, should we set it to the maximum available height? What if no space at all is left?

I think solving all these issues may be a lot of work that could be better spent on the tabbed panel solution if that is the final goal. I'm willing to work in both directions in any case, so just point at what should we do, please :)

@redmunds

This comment has been minimized.

Show comment Hide comment
@redmunds

redmunds Sep 27, 2012

Contributor

This resizing code will still be needed if the tabbed interface is implemented.

Enforcing a minimum height on the editor is just a suggestion for a possible solution, but it's not required. I does not seem to be worth the effort.

But, the mouse not releasing the capture needs to be solved. Can the "dragleave" event be used to release the mouse at the point when mouse exits window?

Contributor

redmunds commented Sep 27, 2012

This resizing code will still be needed if the tabbed interface is implemented.

Enforcing a minimum height on the editor is just a suggestion for a possible solution, but it's not required. I does not seem to be worth the effort.

But, the mouse not releasing the capture needs to be solved. Can the "dragleave" event be used to release the mouse at the point when mouse exits window?

@jbalsas

This comment has been minimized.

Show comment Hide comment
@jbalsas

jbalsas Sep 27, 2012

Contributor

I think dragleave is for drag operations, which we aren't really doing. However, jquery .mouseleave() should do the trick.(Note that this is currently also happening for the sidebar...)

I'll check and fix this and refactor the resize as suggested.

Contributor

jbalsas commented Sep 27, 2012

I think dragleave is for drag operations, which we aren't really doing. However, jquery .mouseleave() should do the trick.(Note that this is currently also happening for the sidebar...)

I'll check and fix this and refactor the resize as suggested.

@jbalsas

This comment has been minimized.

Show comment Hide comment
@jbalsas

jbalsas Oct 1, 2012

Contributor

I've created a a new utils/Resizer as discussed.

It scans the DOM on AppInit.htmlReady for ver-resizable and hor-resizable classes and adds the resize handlers for those elements. It expects a top-resizer or bottom-resizer class in the case of vertical resizing to identify where to put the handler (both could work at the same time in the future). The same way, it expects a left-resizer or right-resizer for horizontal resizing. Finally, it also looks for an optional element with a content-resizable class that should be resized at the same time the parent does, to fill the available space.

The process creates a promise that the resizable modules can use to get notifications of the resizing process using the progress method. The associated deferred objetct never gets resolved. It gets notified any time the resize starts, updates or ends. This way, modules can do performance improvements (like the sidebar does, hiding elements when resizing starts), custom resize on progress, or saving preferences and restoring elements when resize ends.

Known Issues

  • The code works right now for vertical resizing and resizer on top. Bottom vertical resizing as well as horizontal resizing would need small modifications. I think this could be done in future requests.
  • The scan for classes is triggered on AppInit.htmlReady, and modules can retrieve their resize promises on AppInit.appReady. This prevents extensions to benefit from this and force them to explicitly call makeResizable. We could solve this by triggering the scan on AppInit.appReady instead and create a new event Resizer.ready or AppInit.resizerReady to be triggered afterwards.
Contributor

jbalsas commented Oct 1, 2012

I've created a a new utils/Resizer as discussed.

It scans the DOM on AppInit.htmlReady for ver-resizable and hor-resizable classes and adds the resize handlers for those elements. It expects a top-resizer or bottom-resizer class in the case of vertical resizing to identify where to put the handler (both could work at the same time in the future). The same way, it expects a left-resizer or right-resizer for horizontal resizing. Finally, it also looks for an optional element with a content-resizable class that should be resized at the same time the parent does, to fill the available space.

The process creates a promise that the resizable modules can use to get notifications of the resizing process using the progress method. The associated deferred objetct never gets resolved. It gets notified any time the resize starts, updates or ends. This way, modules can do performance improvements (like the sidebar does, hiding elements when resizing starts), custom resize on progress, or saving preferences and restoring elements when resize ends.

Known Issues

  • The code works right now for vertical resizing and resizer on top. Bottom vertical resizing as well as horizontal resizing would need small modifications. I think this could be done in future requests.
  • The scan for classes is triggered on AppInit.htmlReady, and modules can retrieve their resize promises on AppInit.appReady. This prevents extensions to benefit from this and force them to explicitly call makeResizable. We could solve this by triggering the scan on AppInit.appReady instead and create a new event Resizer.ready or AppInit.resizerReady to be triggered afterwards.
@redmunds

This comment has been minimized.

Show comment Hide comment
@redmunds

redmunds Oct 1, 2012

Contributor

Excellent! This is much better. Now that the high-level design is in place, I'm going to do one last review of the code details.

Contributor

redmunds commented Oct 1, 2012

Excellent! This is much better. Now that the high-level design is in place, I'm going to do one last review of the code details.

@redmunds

View changes

src/language/JSLintUtils.js
@@ -23,7 +23,7 @@
/*jslint vars: true, plusplus: true, devel: true, nomen: true, indent: 4, maxerr: 50 */
-/*global define, $, brackets, JSLINT, PathUtils */
+/*global define, $, JSLINT, PathUtils, document, window */

This comment has been minimized.

Show comment Hide comment
@redmunds

redmunds Oct 1, 2012

Contributor

It looks like both document and window are no longer used and can be removed

@redmunds

redmunds Oct 1, 2012

Contributor

It looks like both document and window are no longer used and can be removed

@redmunds

View changes

src/search/FindInFiles.js
@@ -22,7 +22,7 @@
*/
/*jslint vars: true, plusplus: true, devel: true, nomen: true, regexp: true, indent: 4, maxerr: 50 */
-/*global define, $, PathUtils, window */
+/*global define, $, PathUtils, window, document */

This comment has been minimized.

Show comment Hide comment
@redmunds

redmunds Oct 1, 2012

Contributor

It looks like both document and window are no longer used and can be removed

@redmunds

redmunds Oct 1, 2012

Contributor

It looks like both document and window are no longer used and can be removed

@redmunds

View changes

src/utils/Resizer.js
@@ -0,0 +1,200 @@
+/*jslint vars: true, plusplus: true, devel: true, nomen: true, indent: 4, maxerr: 50 */
+/*global define, $, document, window */

This comment has been minimized.

Show comment Hide comment
@redmunds

redmunds Oct 1, 2012

Contributor

Instead of using "document", we generally try to use "window.document", then you don't have to list it as a global.

@redmunds

redmunds Oct 1, 2012

Contributor

Instead of using "document", we generally try to use "window.document", then you don't have to list it as a global.

@redmunds

View changes

src/language/JSLintUtils.js
+ $jslintContent = $("#jslint-results .table-container");
+
+ $jslintResults.height(height);
+ $jslintContent.height(height - 27);

This comment has been minimized.

Show comment Hide comment
@redmunds

redmunds Oct 1, 2012

Contributor

27 seems to be the header height and should be defined in a constant. Same comment for the FiF panel.

@redmunds

redmunds Oct 1, 2012

Contributor

27 seems to be the header height and should be defined in a constant. Same comment for the FiF panel.

@redmunds

View changes

src/styles/brackets.less
+ height: 100%;
+ &.ver-resizing a, &.ver-resizing #projects a, &.ver-resizing .main-view, &.ver-resizing .CodeMirror-lines {
+ cursor: row-resize;
+ }

This comment has been minimized.

Show comment Hide comment
@redmunds

redmunds Oct 1, 2012

Contributor

Brackets currently doesn't support Firefox, so is this necessary? Maybe the comment is incorrect.

@redmunds

redmunds Oct 1, 2012

Contributor

Brackets currently doesn't support Firefox, so is this necessary? Maybe the comment is incorrect.

@@ -0,0 +1,200 @@
+/*jslint vars: true, plusplus: true, devel: true, nomen: true, indent: 4, maxerr: 50 */

This comment has been minimized.

Show comment Hide comment
@redmunds

redmunds Oct 1, 2012

Contributor

Copy the Adobe copyright header comment from any other .js file and paste it at the top.

@redmunds

redmunds Oct 1, 2012

Contributor

Copy the Adobe copyright header comment from any other .js file and paste it at the top.

@redmunds

View changes

src/utils/Resizer.js
+ *
+ * On initialization, Resizer discovers all nodes tagged as "ver-resizable"
+ * and "hor-resizable" to add the resizer handler. Additionally, "top-resizer",
+ * "bottom-resizer", "left-resizer" and "right-resizer" classes control de

This comment has been minimized.

Show comment Hide comment
@redmunds

redmunds Oct 1, 2012

Contributor

typo "de" should be "the"

@redmunds

redmunds Oct 1, 2012

Contributor

typo "de" should be "the"

@redmunds

View changes

src/utils/Resizer.js
+ * @param {DOMNode} element Html element which should be made resizable.
+ * @param {string} direction The direction of the resize action. Must be "hor" or "ver".
+ * @param {string} position The position of the resizer on the element. Can be "top" or "bottom"
+ * for vertical resizing and "left" or "right" for horizontal resizing.

This comment has been minimized.

Show comment Hide comment
@redmunds

redmunds Oct 1, 2012

Contributor

Nit: please indent for readability

@redmunds

redmunds Oct 1, 2012

Contributor

Nit: please indent for readability

@redmunds

View changes

src/utils/Resizer.js
+ * for vertical resizing and "left" or "right" for horizontal resizing.
+ * @param {int} minSize Minimum size (width or height) of the element.
+ * @return {$.Promise} jQuery Promise object that is never resolved, but gets
+ * notified anytime the resize starts, updates or ends.

This comment has been minimized.

Show comment Hide comment
@redmunds

redmunds Oct 1, 2012

Contributor

Nit: please indent for readability

@redmunds

redmunds Oct 1, 2012

Contributor

Nit: please indent for readability

@redmunds

View changes

src/utils/Resizer.js
+ contentSizeFunction = null;
+
+ minSize = minSize || 0;
+ resizePromises[$element.attr("id")] = $deferred.promise();

This comment has been minimized.

Show comment Hide comment
@redmunds

redmunds Oct 1, 2012

Contributor

Having a promise for each resizable panel is an interesting technique. Logically it makes sense, but I am wondering what are the memory and performance implications? Most people will never resize panels. Of those that do, I suspect most will resize them once and never change the size again. Can the promise be created on mouse down and then resolved on mouse up?

@redmunds

redmunds Oct 1, 2012

Contributor

Having a promise for each resizable panel is an interesting technique. Logically it makes sense, but I am wondering what are the memory and performance implications? Most people will never resize panels. Of those that do, I suspect most will resize them once and never change the size again. Can the promise be created on mouse down and then resolved on mouse up?

This comment has been minimized.

Show comment Hide comment
@jbalsas

jbalsas Oct 2, 2012

Contributor

I tried that in the beginning, but then the modules aren't aware of the promise and we'd need to create some mechanism to deliver it to them everytime a new promise is created which I didn't really like and couldn't really figure out a nice way of doing it...

What about having one unique promise for the Resizer module? We could append the resized element id to the parameters when notifying the deferred object. This way, Resizer.resizing would return always the same promise, and the modules could identify which updates refer to them by checking the id argument. (Granted this could get messy if people using the promise aren't careful enough...)

I'm attending tomorrows Hackaton in London, and as I'm no real expert in promises, maybe someone there could pitch in with some ideas?

@jbalsas

jbalsas Oct 2, 2012

Contributor

I tried that in the beginning, but then the modules aren't aware of the promise and we'd need to create some mechanism to deliver it to them everytime a new promise is created which I didn't really like and couldn't really figure out a nice way of doing it...

What about having one unique promise for the Resizer module? We could append the resized element id to the parameters when notifying the deferred object. This way, Resizer.resizing would return always the same promise, and the modules could identify which updates refer to them by checking the id argument. (Granted this could get messy if people using the promise aren't careful enough...)

I'm attending tomorrows Hackaton in London, and as I'm no real expert in promises, maybe someone there could pitch in with some ideas?

This comment has been minimized.

Show comment Hide comment
@redmunds

redmunds Oct 3, 2012

Contributor

The way that comes to mind is to create some custom events (e.g. panelResizeStart, panelResize, panelResizeEnd) that the modules could listen for.

@redmunds

redmunds Oct 3, 2012

Contributor

The way that comes to mind is to create some custom events (e.g. panelResizeStart, panelResize, panelResizeEnd) that the modules could listen for.

@redmunds

View changes

src/utils/Resizer.js
+ // makeResizable(element, DIRECTION_HORIZONTAL, POSITION_LEFT, DEFAULT_MIN_SIZE);
+ //}
+
+ //if ($(element).hasClass("bottom-resizer")) {

This comment has been minimized.

Show comment Hide comment
@redmunds

redmunds Oct 1, 2012

Contributor

Should this be "right-resizer" ?

@redmunds

redmunds Oct 1, 2012

Contributor

Should this be "right-resizer" ?

@redmunds

This comment has been minimized.

Show comment Hide comment
@redmunds

redmunds Oct 1, 2012

Contributor

Done with review.

Contributor

redmunds commented Oct 1, 2012

Done with review.

jbalsas added some commits Oct 3, 2012

Merge branch 'master' of https://github.com/adobe/brackets into jslin…
…t-panel-resize

Conflicts:
	src/brackets.js
	src/language/JSLintUtils.js
	src/search/FindInFiles.js
@redmunds

This comment has been minimized.

Show comment Hide comment
@redmunds

redmunds Oct 8, 2012

Contributor

@jbalsas I see you pushed some changes since my last comment, so I am just checking to see where we're at with this one. I want to get this change into master so I can use it! Also, there are conflicts with master, so you'll need to merge the latest changes to master to this branch. Thanks.

Contributor

redmunds commented Oct 8, 2012

@jbalsas I see you pushed some changes since my last comment, so I am just checking to see where we're at with this one. I want to get this change into master so I can use it! Also, there are conflicts with master, so you'll need to merge the latest changes to master to this branch. Thanks.

@jbalsas

This comment has been minimized.

Show comment Hide comment
@jbalsas

jbalsas Oct 8, 2012

Contributor

@redmunds Yes, I added your final suggestions the other day in London (@peterflynn helped me with those ;)) Please, tell me if there's anything left to add.

I'll merge the changes to master later tonight.

Contributor

jbalsas commented Oct 8, 2012

@redmunds Yes, I added your final suggestions the other day in London (@peterflynn helped me with those ;)) Please, tell me if there's anything left to add.

I'll merge the changes to master later tonight.

Merge branch 'master' of https://github.com/adobe/brackets into jslin…
…t-panel-resize

Conflicts:
	src/search/FindInFiles.js
@redmunds

This comment has been minimized.

Show comment Hide comment
@redmunds

redmunds Oct 8, 2012

Contributor

@jbalsas Glad I asked :) Please post a comment with @redmunds when passing something back so I don't wait unnecessarily.

This looks good. 2 very minor other changes:

  • line 190 of Resizer.js has a typo -- it should be "right" not "bottom".
  • Totally optional, but do you mind renaming "ver-resizable" to be "vert-resizable", and "hor-resizable" to be "horz-resizable"? Seems much quicker to remember what they're for.
Contributor

redmunds commented Oct 8, 2012

@jbalsas Glad I asked :) Please post a comment with @redmunds when passing something back so I don't wait unnecessarily.

This looks good. 2 very minor other changes:

  • line 190 of Resizer.js has a typo -- it should be "right" not "bottom".
  • Totally optional, but do you mind renaming "ver-resizable" to be "vert-resizable", and "hor-resizable" to be "horz-resizable"? Seems much quicker to remember what they're for.
@jbalsas

This comment has been minimized.

Show comment Hide comment
@jbalsas

jbalsas Oct 8, 2012

Contributor

@redmunds Done and done ;)

Contributor

jbalsas commented Oct 8, 2012

@redmunds Done and done ;)

@redmunds

This comment has been minimized.

Show comment Hide comment
@redmunds

redmunds Oct 8, 2012

Contributor

Looks good. Thanks for sticking with this one! Merging.

Contributor

redmunds commented Oct 8, 2012

Looks good. Thanks for sticking with this one! Merging.

redmunds added a commit that referenced this pull request Oct 8, 2012

@redmunds redmunds merged commit 9de1238 into adobe:master Oct 8, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment