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

Find in Files Improvements (Part 2: Pagination) #4303

Merged
merged 11 commits into from Aug 8, 2013

Conversation

@TomMalbran
Copy link
Contributor

commented Jun 20, 2013

This is the second incremental change from #3151 which mainly only adds Pagination and Double clicking on a file to add it to the working set.

Each page now always shows 100 results as before, and the new links More and Less allow to see the next results or the previous ones. This can be changed if required. I also incremented the maximum amount of results per file to 300 since we can now see more than 100 results, but this number was randomly chosen so it can be modified. The collapsed folders are also restored when switching pages.

TomMalbran added 2 commits Jun 20, 2013
@TomMalbran

This comment has been minimized.

Copy link
Contributor Author

commented Jul 15, 2013

Any updates here? It looks like, maybe @larz0 didn't got notified, since assigning doesn't sends an email. Let me know if I should merge with master and fix the conflict issue too.

@larz0

This comment has been minimized.

Copy link
Member

commented Jul 15, 2013

@TomMalbran I'm looking for the "more" and "less" links but can't find them. Here's a screenshot:

screen shot 2013-07-15 at 2 53 13 pm

@TomMalbran

This comment has been minimized.

Copy link
Contributor Author

commented Jul 15, 2013

Hi @larz0. Thanks for looking into this. There was a merge conflict before. I think is what caused you to not see my changes. I just merged with master and fixed the conflict issue. I tested it, and this is how it should look:

alt text

Could you test again with the latest commit? Thanks.

@larz0

This comment has been minimized.

Copy link
Member

commented Jul 16, 2013

This is rad @TomMalbran. While we're at it, we could probably simplify the panel title by making it less verbose:

"t" found — Over 318296 matches in 2846 files < 101 — 200 > (with < and > being arrows, we could use the jsTreeSprite.svg)

Thoughts?

@TomMalbran

This comment has been minimized.

Copy link
Contributor Author

commented Jul 16, 2013

Still can't see them? It is strange if you can't. Are you at my branch?

That sounds good. Not sure if it would be clear enough, but is understandable. Right now jsTreeSprite.svg only has down and right arrows, so we would need a left one. Maybe rename that file to arrows too.

What should we do when showing the first 100 results and the last 100? I think that having a disabled left/right arrow would be the best to avoid changing the position of the arrows. Or maybe a blank space with the same size as the arrow.

@TomMalbran

This comment has been minimized.

Copy link
Contributor Author

commented Jul 16, 2013

We should add the scope in the Result title, since that can change, when there is one. So in that case it could be:

"t" found in src/ — Over 318296 matches in 2846 files — < 101 — 200 >

@larz0

This comment has been minimized.

Copy link
Member

commented Jul 16, 2013

Awesome. Sounds good. I just realized that number ranges shouldn't have any spacing on both sides of the em dash so it should be "< 101—200 >".

@larz0

This comment has been minimized.

Copy link
Member

commented Jul 16, 2013

Also, maybe we should replace the em dash after "files" with ":" like:

"t" found in src/ — Over 318296 matches in 2846 files: < 101—200 >

@TomMalbran

This comment has been minimized.

Copy link
Contributor Author

commented Jul 16, 2013

That looks better. Any suggestions as to what to do with:

"t" found in src/ — Over 318296 matches in 2846 files: x 0—100 >

Where x could be nothing, a disabled arrow or a blank image/space.

@larz0

This comment has been minimized.

Copy link
Member

commented Jul 16, 2013

Just do an opacity 0.3 on the arrow that can't be clicked.

@TomMalbran

This comment has been minimized.

Copy link
Contributor Author

commented Jul 16, 2013

Ok, great. One final thing before implementing this changes. Do you have the assets for a left pointing triangle, since jsTreeSprite.svg doesn't have one?

@larz0

This comment has been minimized.

Copy link
Member

commented Jul 16, 2013

Try this on the sprite:

-webkit-transform: translateZ(0) rotate(180deg);

Not sure if it's 180deg. Also, translateZ(0) is needed to stop it from blurring.

@TomMalbran

This comment has been minimized.

Copy link
Contributor Author

commented Jul 16, 2013

Sure, that should work, although in the long term having all 4 arrows in an svg might be better.

@larz0

This comment has been minimized.

Copy link
Member

commented Jul 16, 2013

Sounds good, I'll create a new task for myself.

@TomMalbran

This comment has been minimized.

Copy link
Contributor Author

commented Jul 17, 2013

@larz0 Design changes done. Let me know how it looks now.

@larz0

This comment has been minimized.

Copy link
Member

commented Jul 17, 2013

Looks good! I'll get someone to review the code now. Thanks @TomMalbran.

@JeffryBooher

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2013

I started my initial review but I need to read through the changes a few more times... I have meetings most of the day today so I may not get back to it until later tonight or tomorrow.

@larz0

This comment has been minimized.

Copy link
Member

commented Jul 18, 2013

Cool @JeffryBooher thanks for the heads up.

if (event.keyCode === KeyEvent.DOM_VK_ESCAPE) {
query = null;
$searchField
.bind("keydown", function (event) {

This comment has been minimized.

Copy link
@JeffryBooher

JeffryBooher Jul 19, 2013

Contributor

why is there a newline before the call to bind()? looks awkward it looks like that was the only change to this block other than the indentation shifted to account for the bind moving to a new line.

This comment has been minimized.

Copy link
@TomMalbran

TomMalbran Jul 19, 2013

Author Contributor

Is just a Style thing. It makes the next chained method looks better. Instead of having:

})
    .bind(function () {

Where the second chained methods goes on the next line and indented. It looks now like:

    })
    .bind(function () {
searchResults.forEach(function (item) {
var numFiles = 0, numMatches = 0;
CollectionUtils.forEach(searchResults, function (item) {
numFiles++;

This comment has been minimized.

Copy link
@JeffryBooher

JeffryBooher Jul 19, 2013

Contributor

minor optimization nit but you could just do numFiles = object.keys(searchResults).length before the forEach. instead of bumping a counter by one on each iteration.

This comment has been minimized.

Copy link
@JeffryBooher

JeffryBooher Jul 19, 2013

Contributor

actually i take that back. it's probably slower and since you need to iterate over the map already this is fine. so disregard.

numMatches += item.matches.length;
});

// No more pages to show
if (currentStart > numMatches || currentStart < 0) {

This comment has been minimized.

Copy link
@JeffryBooher

JeffryBooher Jul 19, 2013

Contributor

is this just a extra safety check or can we get into a situation where currentStart < 0 or > numMatches? Seems like it shouldn't be a case that we need to check but if it's a valid case then shouldn't we do something besides just return? I remember early versions of the find in files panel would be blank but there would be 1 result that you couldn't see. Just trying to understand if you ran into a case where this happened with this code and this check is trying to prevent that.

This comment has been minimized.

Copy link
@TomMalbran

TomMalbran Jul 19, 2013

Author Contributor

Is just a safety check which I haven't run into, since the on click event for the arrows is not added when there isn't a previous or next page. So currentStart shouldn't be less than 0 or more than the max.

If we keep it, you are right, if the FindInFiles panels isn't visible, then it shouldn't return, but if it is visible, then we can just leave the last page.

This comment has been minimized.

Copy link
@TomMalbran

TomMalbran Jul 19, 2013

Author Contributor

I removed this check. We can re-add it if we find a case where this check is required.

@JeffryBooher

This comment has been minimized.

Copy link
Contributor

commented Aug 5, 2013

@TomMalbran The code for this looks good, it looks like all of the changes made it in. I'm unable to test it however because you need to merge master into your branch to deal with the jslint submodule changeover. Can you merge master and I'll take another look and merge this in. We'd like to get this in for this sprint.

@larz0

This comment has been minimized.

Copy link
Member

commented Aug 5, 2013

I can't wait!

@TomMalbran

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2013

@JeffryBooher done :)

@TomMalbran

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2013

It looks like the jsTree Sprite changed, so I fixed it.

@larz0 I just noticed that when there is only 1 page, no arrows are show, but the title still has the : at the end, which look bad. Any suggestion about what to do? I was thinking of just showing the arrows and the results displayed, and both arrows disabled, or replace the : with a . or maybe with nothing.

@TomMalbran TomMalbran referenced this pull request Aug 6, 2013
@larz0

This comment has been minimized.

Copy link
Member

commented Aug 6, 2013

@TomMalbran ahh yes. Let's remove the ":".

@TomMalbran

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2013

@larz0 will do.

@JeffryBooher

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2013

@larz0 I think we need first / last page affordances. We have the data so it should be simple to implement. I searched for "var" (without the quotes) on brackets source and there were 26001 matches... Takes awhile to get to the beginning / end using next / prev page...

@TomMalbran

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2013

We will need an << and a >> icons (or similar), and probably smaller than the current arrows. Or we can add an input to write the page/match number?

@larz0

This comment has been minimized.

Copy link
Member

commented Aug 6, 2013

@JeffryBooher can we merge this for now and update it later? I'll need to create icons for the new affordances. (Don't search for "var" for now :p)

@TomMalbran

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2013

It is possible to create a >> icon with the current arrows SVG in CSS (if that is a good icon to use) and then replace them later with the final design.

@larz0

This comment has been minimized.

Copy link
Member

commented Aug 6, 2013

Here you go @TomMalbran, I've updated the sprite with new icons http://cl.ly/0Y0S252T2S1E

@TomMalbran

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2013

Awesome. Thanks.

@TomMalbran

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2013

Done. Added the go to first and last pages links.

@larz0

This comment has been minimized.

Copy link
Member

commented Aug 7, 2013

@TomMalbran awesome. Could you add "margin: 0 3px;" to the icons to space it up a bit?

@TomMalbran

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2013

margin: 0 3px; gives ends in 6px between 2 arrows, it looks like a bit too much? 1px looks good, and 2px too.

@larz0

This comment has been minimized.

Copy link
Member

commented Aug 7, 2013

@TomMalbran alright let's use 2px.

@TomMalbran

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2013

@larz0 done

@larz0

This comment has been minimized.

Copy link
Member

commented Aug 7, 2013

Looking at @JeffryBooher now :]

@JeffryBooher

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2013

All tests pass...merging

JeffryBooher added a commit that referenced this pull request Aug 8, 2013
Find in Files Improvements (Part 2: Pagination)
@JeffryBooher JeffryBooher merged commit 1fb9041 into adobe:master Aug 8, 2013
1 check passed
1 check passed
default The Travis CI build passed
Details
@TomMalbran TomMalbran deleted the TomMalbran:tom/find-in-files-p2 branch Aug 8, 2013
@TomMalbran

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2013

Woot. Next part will be awesome too :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.