Gave search result panel a min-height#5391
Conversation
ghost
assigned
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
@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 => 100So I think the default value should be 0.
(I'm new here and want to learn so I am open to any solution)
There was a problem hiding this comment.
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;
}
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Now that default panel height is 100, it's no longer necessary to pass 100 here.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
It's ok to leave it in, it's just not "necessary". I don't think it's worth updating that other code.
There was a problem hiding this comment.
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.
|
Looks good. Just a couple minor comments. |
There was a problem hiding this comment.
Is this really a good comment? I refer to a variable that exist in another class/file. But like you said: DEFAULT_MIN_SIZE is not part of the public API and should not be referred through "Resizer.DEFAULT_MIN_SIZE". However "Resizer.DEFAULT_MIN_SIZE" is accessible in this file so maybe it should be better to refer to "Resizer.DEFAULT_MIN_SIZE" is this case?
Sorry if I creating extra work for you but this (simple) bug will soon be dead :)
There was a problem hiding this comment.
I agree that this file should not refer to DEFAULT_MIN_SIZE. The sentence "default is [whatever]" should either be removed or maybe changed to "default is set in Resizer.makeResizable()".
|
I understand if you haven't had time to look at this issue, but just to be clear, is the code good now? (just to clarify that you don't wait for me). |
|
Sorry about the delay -- code looks good. We stopped merging code while we closed down Sprint 32, so I'll be merging this soon. |
|
Looks good. Merging. |
Gave search result panel a min-height
2 participants
Gave "search-results" panel a min-height, just like the "replace-all-results" and "problems-panel" already has.
Issue: #5175