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
merged 5 commits into from
Oct 9, 2013
Merged

Gave search result panel a min-height #5391

Changes from 1 commit
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
Contributor

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
Contributor Author

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
Contributor

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
Contributor Author

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
Contributor

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
Contributor Author

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
Contributor

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