Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
core-ai-bot opened this issue Aug 30, 2021 · 26 comments
Open

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

core-ai-bot opened this issue Aug 30, 2021 · 26 comments

Comments

@core-ai-bot
Copy link
Member

Issue by sathyamoorthi
Wednesday Oct 30, 2013 at 05:46 GMT
Originally opened as adobe/brackets#5757


Based on this Google groups discussion

CC: @larz0,@njx,@TomMalbran


sathyamoorthi included the following code: https://github.com/adobe/brackets/pull/5757/commits

@core-ai-bot
Copy link
Member Author

Comment by MarcelGerber
Wednesday Oct 30, 2013 at 14:38 GMT


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

@core-ai-bot
Copy link
Member Author

Comment by larz0
Wednesday Oct 30, 2013 at 21:33 GMT


@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?

@core-ai-bot
Copy link
Member Author

Comment by sathyamoorthi
Thursday Oct 31, 2013 at 06:35 GMT


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

@core-ai-bot
Copy link
Member Author

Comment by larz0
Thursday Oct 31, 2013 at 19:24 GMT


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

screen shot 2013-10-31 at 12 23 38 pm

@core-ai-bot
Copy link
Member Author

Comment by sathyamoorthi
Friday Nov 01, 2013 at 00:41 GMT


@larz0 small change. added that.

@core-ai-bot
Copy link
Member Author

Comment by sathyamoorthi
Friday Nov 22, 2013 at 09:09 GMT


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

@core-ai-bot
Copy link
Member Author

Comment by larz0
Sunday Jan 19, 2014 at 00:00 GMT


@JeffryBooher PRETTY PLEASE WITH 🍉 ON TOP~

@core-ai-bot
Copy link
Member Author

Comment by TomMalbran
Sunday Jan 19, 2014 at 00:21 GMT


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.

@core-ai-bot
Copy link
Member Author

Comment by larz0
Sunday Jan 19, 2014 at 00:46 GMT


Thanks@TomMalbran!

@core-ai-bot
Copy link
Member Author

Comment by sathyamoorthi
Monday Jan 20, 2014 at 03:25 GMT


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

@core-ai-bot
Copy link
Member Author

Comment by TomMalbran
Monday Jan 20, 2014 at 03:29 GMT


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.

@core-ai-bot
Copy link
Member Author

Comment by sathyamoorthi
Monday Jan 20, 2014 at 03:44 GMT


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

@core-ai-bot
Copy link
Member Author

Comment by TomMalbran
Wednesday Jan 22, 2014 at 19:28 GMT


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

@core-ai-bot
Copy link
Member Author

Comment by sathyamoorthi
Thursday Jan 23, 2014 at 08:55 GMT


@TomMalbran Please do another review.

@core-ai-bot
Copy link
Member Author

Comment by TomMalbran
Thursday Jan 23, 2014 at 19:46 GMT


@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.

@core-ai-bot
Copy link
Member Author

Comment by sathyamoorthi
Friday Jan 24, 2014 at 03:18 GMT


@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 (adobe/brackets#5866, adobe/brackets#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.

@core-ai-bot
Copy link
Member Author

Comment by TomMalbran
Friday Jan 24, 2014 at 06:32 GMT


@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.

@core-ai-bot
Copy link
Member Author

Comment by sathyamoorthi
Friday Jan 24, 2014 at 10:42 GMT


_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.

@core-ai-bot
Copy link
Member Author

Comment by sathyamoorthi
Friday Jan 24, 2014 at 10:43 GMT


_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.

@core-ai-bot
Copy link
Member Author

Comment by larz0
Friday Jan 24, 2014 at 12:48 GMT


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.

@core-ai-bot
Copy link
Member Author

Comment by sathyamoorthi
Friday Jan 24, 2014 at 13:09 GMT


@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?

@core-ai-bot
Copy link
Member Author

Comment by TomMalbran
Friday Jan 24, 2014 at 21:00 GMT


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.

@core-ai-bot
Copy link
Member Author

Comment by peterflynn
Saturday Jan 25, 2014 at 00:40 GMT


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/

@core-ai-bot
Copy link
Member Author

Comment by njx
Saturday Jan 25, 2014 at 00:43 GMT


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.

@core-ai-bot
Copy link
Member Author

Comment by TomMalbran
Saturday Jan 25, 2014 at 01:12 GMT


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

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

@core-ai-bot
Copy link
Member Author

Comment by sathyamoorthi
Saturday Jan 25, 2014 at 04:23 GMT


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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant