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

Added the search feature on the Server Files fragment. #36

Merged
merged 6 commits into from
Mar 17, 2017

Conversation

octacode
Copy link
Member

@octacode octacode commented Mar 7, 2017

fixes #35
Please have a look.
screenshot_20170308-020753
screenshot_20170308-020805

@octacode
Copy link
Member Author

octacode commented Mar 8, 2017

Should we add this search feature in shares fragment too?? Any suggestions?

@cpg
Copy link
Member

cpg commented Mar 8, 2017

Typically there are not many shares in users' home servers, just a handful, so I would say no. Same with apps. Files .. usually there can be a lot.

We probably should hook up this search to an API in the back-end server, so that the WHOLE content of a share can be searched easily. Now that would be useful!

Can you propose this idea, proposing an API (and faking it in development), in a separate issue?
My guess would be search function to the api, however, how does one represent the full path to the file and how does one access to it, it's a fundamental change from what we have now with hierarchical navigation. Say I search for "tax" and there are multiple files deep in multiple folders .. navigating that would have to be implemented differently than what we have now ..

@octacode
Copy link
Member Author

octacode commented Mar 8, 2017

Ya....hooking up the search with an API would be awesome. But for instance if I open a folder say pictures and there are hundreds of pictures in it and I want a picture name starting with z then I have to scroll the whole way down. With this feature just type search on the search toolbar all the items with z will be there. However nested search isn't being implememted here. But still it can come in handy. User can search for the folder and inside the folder he/she will get the file he/she is looking for.

@octacode
Copy link
Member Author

octacode commented Mar 8, 2017

@cpg....I was thinking of displaying both the offline (the results of this PR) and the API search results at the same time. I have seen other apps doing it for example MusixMatch. We will have a single list. First all offline results will be displayed and after that the API results Wii be shown. Somewhat like this
screenshot_20170308-171542

The first one is an offline results while the others are fetched from their API

@cpg
Copy link
Member

cpg commented Mar 8, 2017

Please move this new discussion on what to do for the API, etc., to a new issue.

I have tried this PR and I was able to search (nice), however, opening a file or two, then tapping the back button twice seems to crash the app. I was not able to get a log on the crash, however.

@octacode octacode closed this Mar 8, 2017
@octacode octacode reopened this Mar 8, 2017
@octacode
Copy link
Member Author

octacode commented Mar 8, 2017

@cpg Thanks for reporting the bug. I have fixed it. Please have a look.

@cpg
Copy link
Member

cpg commented Mar 14, 2017

This feature is great, however, the UI for it is not great. Here are some suggestions to improve it:

  • The placeholder says something like "Enter name of file". It should be best to just be "Search"
  • The cursor and the search string is centered, but it should be left-aligned
  • The title of the share disappears. To make this feature more user-friendly, I would suggest that the logo disappear as well to make space for the search term
  • The search term turns to black color with grey background. This looks weird and it may not be very conforming of the latest guidelines.

For all of the above, I would suggest looking at what the Gmail app does and get inspiration from it, as it's probably a faithful implementation of the guidelines.

@cpg cpg assigned cpg and octacode Mar 14, 2017
@octacode
Copy link
Member Author

octacode commented Mar 14, 2017

@cpg I will incorporate the changes. ☺️
There's one more thing can you please mention the steps of recreating the step 4. Thanks.

@octacode octacode force-pushed the search branch 2 times, most recently from 8466360 to b2935fa Compare March 14, 2017 12:33
@cpg
Copy link
Member

cpg commented Mar 14, 2017

Much better. Great job.

I did not have to do anything to step 4. I merged this PR branch into my master (which is ToT at the moment) and it displays like that. If I touch on the text, the back/grey goes away, and a cursor with a circle at the bottom comes in (which is hardly visible, as it blends with the blue bg). I am using an Android 6 device.

@octacode
Copy link
Member Author

@cpg Thanks for the details. I have now changed the cursor and the cursor pointer color to white so that the cursor is easily visible. Moreover I have set the longClickable false. I hope this solves the issue.

@cpg
Copy link
Member

cpg commented Mar 15, 2017

This is what I see in the search box

@octacode
Copy link
Member Author

@cpg I tried this in different phones but wasn't able to replicate the same. I guess it's something to do with the phone settings. I would like others also to take a look at it. @megabitdragon , @csoni111 , @sukhbir-singh , @mehtamanan0 .
Thanks

@cpg cpg merged commit 0ffc199 into amahi:master Mar 17, 2017
@octacode
Copy link
Member Author

@cpg Thanks for the merge.

@octacode octacode deleted the search branch March 17, 2017 10:46
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

Successfully merging this pull request may close these issues.

2 participants