Skip to content
This repository was archived by the owner on Sep 6, 2021. It is now read-only.

Gave search result panel a min-height #5391

Merged
redmunds merged 5 commits into from
Oct 9, 2013
Merged

Gave search result panel a min-height #5391

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
fabaa6b
Gave searchPanel a min height
tjoskar Oct 1, 2013
429ae86
Changed default minSize from 0 to 100 for Resizer.makeResizable()
tjoskar Oct 2, 2013
4e2ff45
Changed comments that are related to minSize for creating panels
tjoskar Oct 3, 2013
f2bd6b2
Modified comment
tjoskar Oct 3, 2013
5aff268
Missed one comment
tjoskar Oct 3, 2013
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/search/FindInFiles.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -848,7 +848,7 @@ define(function (require, exports, module) {
// Initialize items dependent on HTML DOM
AppInit.htmlReady(function () {
var panelHtml = Mustache.render(searchPanelTemplate, Strings);
searchResultsPanel = PanelManager.createBottomPanel("find-in-files.results", $(panelHtml));
searchResultsPanel = PanelManager.createBottomPanel("find-in-files.results", $(panelHtml), 100);
Copy link
Copy Markdown
Contributor

@redmunds redmunds Oct 1, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this fixes the problem. But, Resizer module used to set 100 as the default minimum size -- not sure when or why that changed.

It seems like a more complete solution would be to change Resizer.js line 166 from:

    minSize = minSize || 0;

to

    minSize = minSize || 100;

@peterflynn Since you commented on the bug, I'll ask you: can you think of any case that this would not be desirable?

Copy link
Copy Markdown
Contributor Author

@tjoskar tjoskar Oct 2, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@redmunds I agree,

minSize = minSize || 100;

seems to be safer but I believe that

minSize = minSize || 0;

is a more general design. Furthermore, if the default value is 100 it is not posible to hide elements by dragging (e.g. the side panel) because:

0 || 100 => 100

So I think the default value should be 0.

(I'm new here and want to learn so I am open to any solution)

Copy link
Copy Markdown
Contributor

@redmunds redmunds Oct 2, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! I think the default should be 100, but any panel should be able to override this with 0 (although dragging size to 0 is not a good way to hide panel as is shown in the bug), so this should be:

if (minSize === undefined) {
    minSize = 100;
}

Copy link
Copy Markdown
Contributor Author

@tjoskar tjoskar Oct 2, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, but isn't it better to use DEFAULT_MIN_SIZE instead of a hard coded integer? (see comment below: src/utils/Resizer.js:167)

Copy link
Copy Markdown
Contributor

@redmunds redmunds Oct 2, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that default panel height is 100, it's no longer necessary to pass 100 here.

Copy link
Copy Markdown
Contributor Author

@tjoskar tjoskar Oct 3, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, but if we don't pass 100 as an argument here we shouldn't do that on the error panel (https://github.com/adobe/brackets/blob/master/src/language/CodeInspection.js#L387) nor the find-replace-panel (https://github.com/adobe/brackets/blob/master/src/search/FindReplace.js#L627) to keep consistency?

Copy link
Copy Markdown
Contributor

@redmunds redmunds Oct 3, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's ok to leave it in, it's just not "necessary". I don't think it's worth updating that other code.


$searchResults = $("#search-results");
$searchSummary = $searchResults.find(".title");
Expand Down
9 changes: 6 additions & 3 deletions src/utils/Resizer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ define(function (require, exports, module) {
* @param {!string} position Which side of the element can be dragged: one of the POSITION_* constants
* (TOP/BOTTOM for vertical resizing or LEFT/RIGHT for horizontal).
* @param {?number} minSize Minimum size (width or height) of the element's outer dimensions, including
* border & padding. Defaults to 0.
* border & padding. Defaults to 100.
Copy link
Copy Markdown
Contributor

@redmunds redmunds Oct 2, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment should say "DEFAULT_MIN_SIZE" in stead of "100" in case the default is ever changed.

Copy link
Copy Markdown
Contributor Author

@tjoskar tjoskar Oct 3, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch!
Is "Resizer.DEFAULT_MIN_SIZE" a better name (instead of just DEFAULT_MIN_SIZE)?

Copy link
Copy Markdown
Contributor

@redmunds redmunds Oct 3, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DEFAULT_MIN_SIZE is not part of the public API, so it should just be DEFAULT_MIN_SIZE.

* @param {?boolean} collapsible Indicates the panel is collapsible on double click on the
* resizer. Defaults to false.
* @param {?string} forceLeft CSS selector indicating element whose 'left' should be locked to the
Expand All @@ -162,8 +162,11 @@ define(function (require, exports, module) {
elementSizeFunction = direction === DIRECTION_HORIZONTAL ? $element.width : $element.height,
resizerCSSPosition = direction === DIRECTION_HORIZONTAL ? "left" : "top",
contentSizeFunction = direction === DIRECTION_HORIZONTAL ? $resizableElement.width : $resizableElement.height;

minSize = minSize || 0;

if (minSize === undefined) {
minSize = DEFAULT_MIN_SIZE;
Copy link
Copy Markdown
Contributor Author

@tjoskar tjoskar Oct 2, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct me if i'm wrong but from what I understand, DEFAULT_MIN_SIZE is defined for this purpose i.e. to set a min-size on a panel.

Copy link
Copy Markdown
Contributor

@redmunds redmunds Oct 2, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct. Good catch!

}

collapsible = collapsible || false;

$element.prepend($resizer);
Expand Down
4 changes: 2 additions & 2 deletions src/view/PanelManager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ define(function (require, exports, module) {
* Represents a panel below the editor area (a child of ".content").
*
* @param {!jQueryObject} $panel The entire panel, including any chrome, already in the DOM.
* @param {number=} minSize Minimum height of panel in px; default is 0
* @param {number=} minSize Minimum height of panel in px; default is 100
*/
function Panel($panel, minSize) {
this.$panel = $panel;
Expand Down Expand Up @@ -187,7 +187,7 @@ define(function (require, exports, module) {
*
* @param {!string} id Unique id for this panel. Use package-style naming, e.g. "myextension.feature.panelname"
* @param {!jQueryObject} $panel DOM content to use as the panel. Need not be in the document yet.
* @param {number=} minSize Minimum height of panel in px; default is 0
* @param {number=} minSize Minimum height of panel in px; default is 100
* @return {!Panel}
*/
function createBottomPanel(id, $panel, minSize) {
Expand Down
⚡