Expand / Collapse All for "Find In Files" result. #5757

sathyamoorthi · 2013-10-30T05:46:24Z

Based on this Google groups discussion

CC: @larz0, @njx, @TomMalbran

marcelgerber · 2013-10-30T14:38:41Z

Maybe we could make this more visible, when we do it like this users have to remember one more trick.

larz0 · 2013-10-30T21:33:53Z

@SAplayer we could use a tooltip "Ctrl/Cmd Click to Expand/Collapse all."

@sathyamoorthi Ctrl click doesn't work for me. Could it be because I'm on Mac OS X?

sathyamoorthi · 2013-10-31T06:35:04Z

@larz0 yes. That was mac only issue. Could you please try now?

larz0 · 2013-10-31T19:24:54Z

@sathyamoorthi nice it works now :) Could you add title attribute to the triangle span so we can get a tooltip? This is awesome.

sathyamoorthi · 2013-11-01T00:41:13Z

@larz0 small change. added that.

sathyamoorthi · 2013-11-22T09:09:17Z

@JeffryBooher any chance to pull this in? only simple changes.

larz0 · 2014-01-19T00:00:01Z

@JeffryBooher PRETTY PLEASE WITH 🍉 ON TOP~

TomMalbran · 2014-01-19T00:08:51Z

src/search/FindInFiles.js

+
+                                    if (searchResults[fullPath].collapsed !== collapsed) {
+                                        $(this).nextUntil(".file-section").toggle();
+                                        $(this).find(".disclosure-triangle").toggleClass("expanded").toggleClass("collapsed");


These 2 lines are almost the sames as lines (454-455), only the element changes. So I was thinking that maybe we could have a function: _collapseFileResults($row, collapse: boolean) that could collapse, or uncollapse the results depending on the boolean parameter. Not sure if we should also change the collapse value of the results object in this function.

Yes, it should change the internal state too, so the function can be _collapseFileResults($row, collapse, fullPath).

TomMalbran · 2014-01-19T00:21:45Z

I totally forgot about this PR. I guess I just did a full code review. Haven't tested it yet since is quite behind master. Could you merge it with master too?

I guess I could revive this PR if @sathyamoorthi is not available.

larz0 · 2014-01-19T00:46:23Z

Thanks @TomMalbran!

sathyamoorthi · 2014-01-20T03:25:57Z

@TomMalbran if you wish, please take this forward. I can't get into this for next 2 to 3 days.

TomMalbran · 2014-01-20T03:29:46Z

You can do it (I was going to do it if I got no response in like a week or 2). We still need to wait for Sprint 36 to end, and we will have like 3 more weeks until Sprint 37 when this could be released, so we have plenty of time.

sathyamoorthi · 2014-01-20T03:44:12Z

@TomMalbran Cool. Let me fix this and get back to you in few days. Thanks.

Define self to prevent exception

TomMalbran · 2014-01-22T19:28:54Z

@JeffryBooher If you are busy with other stuff I can take this PR.

…kets into Find-In-Files Conflicts: src/search/FindInFiles.js

sathyamoorthi · 2014-01-23T08:55:02Z

@TomMalbran Please do another review.

TomMalbran · 2014-01-23T19:38:20Z

src/search/FindInFiles.js

@@ -417,7 +417,8 @@ define(function (require, exports, module) {
            // Insert the search results
            $searchContent
                .empty()
-                .append(Mustache.render(searchResultsTemplate, {searchList: searchList}))
+                .append(Mustache.render(searchResultsTemplate, {searchList: searchList,
+                                                                triangleTitle: Strings.FIND_IN_FILES_EXPAND_COLLAPSE}))


We usually just add the Strings as a key/value of the object and use {{Strings.FIND_IN_FILES_EXPAND_COLLAPSE}} inside the template. In that way we don't need to add more strings and we can know what is a string in the template.

Minor Nit: Could you also add enter after { and before } and align the values?

TomMalbran · 2014-01-23T19:46:06Z

@sathyamoorthi Second pass done. I like how you used the same code to do both collapse actions. I just added 2 nit comments.

It looks like the commits got messed up? There are a few repeated and one from dangoor.

sathyamoorthi · 2014-01-24T03:18:46Z

@TomMalbran I corrected your mentioned nits.

_It looks like the commits got messed up? There are a few repeated and one from dangoor._
I faced these kind of issues in my other PR as well (#5866, #5951).

When I do big merge (2 months data) from upstream, i think github got confused sometimes. But i'm not sure about this either. But this PR should not have any issue, while you pull it in.

TomMalbran · 2014-01-24T06:32:22Z

@sathyamoorthi The code looks great and works.

One other question (maybe to @LarZ too). Should we try to keep the scroll at the title (file) you click when you uncollapse?

Maybe you are doing some sort of rebase? I never got this issue by just doing a fetch and then a merge. Anyway, before merging, do you think you could do a rebase? There are lots of commits and most aren't needed.

sathyamoorthi · 2014-01-24T10:42:57Z

_One other question (maybe to @LarZ too). Should we try to keep the scroll at the title (file) you click when you uncollapse?_

Hmmm. I'm not sure. Let we here it from @LarZ too. Because, when user's intention is to see that one file, they will do regular click. Not ctrl + click. I will say this is good to have feature. But not mandatory. Let we here it from @larz0 (Because he is the other guy interested in this PR)

And i would like to coin the other feature that i think, would be very useful. How about sorting search results based on file names?

_Maybe you are doing some sort of rebase? I never got this issue by just doing a fetch and then a merge. Anyway, before merging, do you think you could do a rebase? There are lots of commits and most aren't needed._

yes i did rebase to my master. I think, something i'm doing wrong while merging upstream->master->branch. If you feel this is an issue to accept this PR, let me close this PR and create new clear PR.

sathyamoorthi · 2014-01-24T10:43:49Z

_One other question (maybe to @LarZ too). Should we try to keep the scroll at the title (file) you click when you uncollapse?_

Hmmm. I'm not sure. Let we here it from @LarZ too. Because, when user's intention is to see that one file, they will do regular click. Not ctrl + click. I will say this is good to have feature. But not mandatory. Let we here it from @larz0 (Because he is the other guy interested in this PR)

And i would like to coin the other feature that i think, would be very useful. How about sorting search results based on file names?

_Maybe you are doing some sort of rebase? I never got this issue by just doing a fetch and then a merge. Anyway, before merging, do you think you could do a rebase? There are lots of commits and most aren't needed._

yes i did rebase to my master. I think, something i'm doing wrong while merging upstream->master->branch. If you feel this is an issue to accept this PR, let me close this PR and create new clear PR.

larz0 · 2014-01-24T12:48:28Z

I won't be able to test it out since I'm on vacation til Feb 7. Let's see what it's like first without adding any extra stuff and then we can decide later if it's needed.

sathyamoorthi · 2014-01-24T13:09:55Z

@larz0 Cool. Agree. Enjoy your vacation...

@TomMalbran Let we wrap this PR here. And if you wish i'll work on additional features and give PR later. Can you merge this PR or do you need me to open separate PR in order to avoid more commits?

TomMalbran · 2014-01-24T21:00:04Z

It does makes sense that if you want to see one file you just open that one, and the scroll will not be an issue. If it becomes an issue, it can be fixed later.

Rebasing is a hard thing to manage and it not always ends up ok. I guess it will not be a problem merging it like this, but if you can rebase it into 1 commit, I think that will be better.

peterflynn · 2014-01-25T00:40:29Z

Another option for making a "cleaner" PR would be to just generate a .patch file for the original branch, make a new clean branch off of master, and then apply the .patch file there. There's a tutorial on creating & applying .patches here, for example: http://ariejan.net/2009/10/26/how-to-create-and-apply-a-patch-with-git/

njx · 2014-01-25T00:43:43Z

Even easier would be to just create a new branch off master, and then git merge --squash thisbranch, then git commit. That will give you a single commit with all the changes from this branch.

TomMalbran · 2014-01-25T01:12:23Z

Thanks for the info. Should I do it or @sathyamoorthi?

BTW, at which point do we request for a rebase?

sathyamoorthi · 2014-01-25T04:23:45Z

@TomMalbran fixing this PR is taking me long time. So here is new clean PR #6640. Please use that and close this.

Expand / Collapse All for "Find In Files" result.

39d9c90

e.metaKey added to support Mac.

7ccde5f

Tooltip added to expand/collapse triangle.

57b9957

ghost assigned JeffryBooher Nov 1, 2013

TomMalbran reviewed Jan 19, 2014
View reviewed changes

Merge pull request #6608 from adobe/nj/issue-6607

ba3e47c

Define self to prevent exception

sathyamoorthi added 8 commits January 23, 2014 08:00

Expand / Collapse All for "Find In Files" result.

ecd1e80

e.metaKey added to support Mac.

939b643

Tooltip added to expand/collapse triangle.

79dea63

coding standard fixes.

409ed84

e.metaKey added to support Mac.

524e8d6

Tooltip added to expand/collapse triangle.

0df4912

coding standard fixes.

5e88b67

Merge branch 'Find-In-Files' of https://github.com/sathyamoorthi/brac…

6e9723b

…kets into Find-In-Files Conflicts: src/search/FindInFiles.js

TomMalbran reviewed Jan 23, 2014
View reviewed changes

sathyamoorthi added 2 commits January 24, 2014 08:29

changes to pass second review.

7c14fe9

changes to pass second review.

fe7fe93

sathyamoorthi mentioned this pull request Jan 25, 2014

Expand / Collapse All for "Find In Files" result - clean commit #6640

Merged

TomMalbran closed this Jan 25, 2014

ghost assigned TomMalbran Jan 25, 2014

This was referenced Aug 30, 2021

[CLOSED] Expand / Collapse All for "Find In Files" result. brackets-archive/bracketsIssues#5279

Open

[CLOSED] Expand / Collapse All for "Find In Files" result - clean commit brackets-archive/bracketsIssues#6032

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expand / Collapse All for "Find In Files" result. #5757

Expand / Collapse All for "Find In Files" result. #5757

sathyamoorthi commented Oct 30, 2013

marcelgerber commented Oct 30, 2013

larz0 commented Oct 30, 2013

sathyamoorthi commented Oct 31, 2013

larz0 commented Oct 31, 2013

sathyamoorthi commented Nov 1, 2013

sathyamoorthi commented Nov 22, 2013

larz0 commented Jan 19, 2014

TomMalbran Jan 19, 2014

TomMalbran Jan 19, 2014

TomMalbran commented Jan 19, 2014

larz0 commented Jan 19, 2014

sathyamoorthi commented Jan 20, 2014

TomMalbran commented Jan 20, 2014

sathyamoorthi commented Jan 20, 2014

TomMalbran commented Jan 22, 2014

sathyamoorthi commented Jan 23, 2014

TomMalbran Jan 23, 2014

TomMalbran commented Jan 23, 2014

sathyamoorthi commented Jan 24, 2014

TomMalbran commented Jan 24, 2014

sathyamoorthi commented Jan 24, 2014

sathyamoorthi commented Jan 24, 2014

larz0 commented Jan 24, 2014

sathyamoorthi commented Jan 24, 2014

TomMalbran commented Jan 24, 2014

peterflynn commented Jan 25, 2014

njx commented Jan 25, 2014

TomMalbran commented Jan 25, 2014

sathyamoorthi commented Jan 25, 2014

Expand / Collapse All for "Find In Files" result. #5757

Expand / Collapse All for "Find In Files" result. #5757

Conversation

sathyamoorthi commented Oct 30, 2013

marcelgerber commented Oct 30, 2013

larz0 commented Oct 30, 2013

sathyamoorthi commented Oct 31, 2013

larz0 commented Oct 31, 2013

sathyamoorthi commented Nov 1, 2013

sathyamoorthi commented Nov 22, 2013

larz0 commented Jan 19, 2014

TomMalbran Jan 19, 2014

Choose a reason for hiding this comment

TomMalbran Jan 19, 2014

Choose a reason for hiding this comment

TomMalbran commented Jan 19, 2014

larz0 commented Jan 19, 2014

sathyamoorthi commented Jan 20, 2014

TomMalbran commented Jan 20, 2014

sathyamoorthi commented Jan 20, 2014

TomMalbran commented Jan 22, 2014

sathyamoorthi commented Jan 23, 2014

TomMalbran Jan 23, 2014

Choose a reason for hiding this comment

TomMalbran commented Jan 23, 2014

sathyamoorthi commented Jan 24, 2014

TomMalbran commented Jan 24, 2014

sathyamoorthi commented Jan 24, 2014

sathyamoorthi commented Jan 24, 2014

larz0 commented Jan 24, 2014

sathyamoorthi commented Jan 24, 2014

TomMalbran commented Jan 24, 2014

peterflynn commented Jan 25, 2014

njx commented Jan 25, 2014

TomMalbran commented Jan 25, 2014

sathyamoorthi commented Jan 25, 2014