search

atom / fuzzy-finder

Find and open files quickly
MIT License
276 stars 138 forks source link

add maxItem as option #198

Closed shemerey closed 8 years ago

shemerey commented 8 years ago

Hi there, can we discuss this PR ?

Sometimes it's nice to see all possible results what can fits on your screen. It's hard to override this hardcoded value. I think it's better to have it in config.

lee-dohm commented 8 years ago

This setting won't change the size of the box that is drawn:

screen shot 2016-04-27 at 4 54 06 pm

It is currently hard coded to 10, but on my theme I only see 5. (I'm not sure if this is controlled by the theme or the package.)

shemerey commented 8 years ago

@lee-dohm it will not by default, but if you want to customize how it looks, you have no options to control, It makes life harder for third party packages what consume FF as well.

I my case I found it handy to open FF on full screen like so, but I can't see all results, and half screen is useful.

FuzzyFinder with hardcoded value

If I can control it by settings I can see something like this

FuzzyFinder with settings

I understand that this is might be rare case but it looks like more pros than cons. can be wrong :)

lee-dohm commented 8 years ago

So having a minimum of 5 and a maximum of 20 should be sufficient then, given your screenshot.

shemerey commented 8 years ago

yes, I think so. I have no idea why I use 80 as max value :)

lee-dohm commented 8 years ago

Sounds good. If you can make that change and add a test so that we don't inadvertently break this functionality in the future, I'd be happy to merge it.

shemerey commented 8 years ago

@lee-dohm plz take a look, when you have time

shemerey commented 8 years ago

@lee-dohm I've added some test according to your review. Since it's a package activation related option I have to introduce new top level describe with customised activation.

plz take a look when you have time, and let me know if you have any thought about it

shemerey commented 8 years ago

@lee-dohm, @50Wliu do you have any suggestions, or any thoughts about current PR ?

shemerey commented 8 years ago

@lee-dohm, @50Wliu ping

winstliu commented 8 years ago

Looks good for the most part. My only issue with this is that the new spec was added at the top of the file in its own top-level describe block, when it should probably go somewhere under the existing FuzzyFinder describe (such as https://github.com/atom/tree-view/blob/46076caaee988972175398e056e7adb14806b8cf/spec/tree-view-spec.coffee#L2456).

lee-dohm commented 8 years ago

Thanks for the hard work and effort that you put in to this. I've reviewed it with the team and we've decided that we don't want to add this change. We're trying to control the complexity of Atom's core functionality to prevent costly regressions.

I apologize that the team didn't have this discussion sooner to save you the time you put into it.