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

issue/62-simple post search #10023

Merged
merged 54 commits into from Jun 25, 2019

Conversation

@khaykov
Copy link
Member

commented Jun 11, 2019

Fixes #62

This PR adds the ability to search published and private sites on jetpack/wpcom sites.

Implementation
I used Pages Search functionality as a reference. Pages Search is using a dedicated search fragment, ViewModels, and adapters. I originally tried to go the same route but realized that this brings a lot of extra code that we will have to maintain alongside existing Post List, so I decided to try and use existing PostListFragment for search (huge mistake, but too late!)

The current implementation of Post List is pretty robust, and meant to work in a "fire and forget" kind of way, so I had to make a couple (maybe a lot) of minor changes to accommodate special behavior of Search list. I tried not to touch the core of PostListViewModel, so the changes are spread through a couple of helper classes and PostListFragment

Search button logic in the toolbar is mostly copied from Pages List, with a couple of minor changes.

UX

  • The search ignores filters in main post list and looks for Published and Private posts for all authors.
  • Search list respects "view mode" selected at the main post list (condensed or full).
  • Delay for search query input is 500 ms.
  • One thing that was not covered in a mock was progress indication. Similar search screens we have in the app are doing local DB search, so there is almost no delay. Post search does a network search, and most of the time it's pretty fast, so really brief progress indication every time you stopped tapping was really annoying. To combat this I added a delay to PTR indicator, that appears if a result has not arrived within 1 second.

search

To test:
Make sure the post Search button is not visible for self-hosted sites without a jetpack.
Make sure post search works as expected.

Update release notes:

  • If there are user facing changes, I have added an item to RELEASE-NOTES.txt.

@khaykov khaykov added this to the 12.7 milestone Jun 11, 2019

@khaykov

This comment has been minimized.

Copy link
Member Author

commented Jun 20, 2019

Thanks for the review, @malinajirka ! Looks like ThrottleLiveData fixed the empty screen in post list :) I implemented a majority of your suggestions and left a couple of comments/questions :)

@malinajirka

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2019

Thank you so much for the changes @khaykov! The code is way simpler now ❤️!

I've tested the search and it works like a charm 🎉.

I've left one last suggestion and one question/suggestion. But I think it's already ready to be merged. Good job:)!

}
connector = postListViewModelConnector

initList(null, dataSource, lifecycle)

This comment has been minimized.

Copy link
@malinajirka

malinajirka Jun 21, 2019

Contributor

I'm sorry for coming back to this, but I'm still not sure I understand why we can't wrap initList in the start with if (connector.postListType != SEARCH) and get rid of both isEmptySearch calls.
We would get rid of the first initial request with empty searchQuery -> which fetches all the published/private posts on the site even though we throw them away. Wdyt?

@SylvesterWilmott

This comment has been minimized.

Copy link

commented Jun 21, 2019

For transparency:

@khaykov and I discussed the design in a private conversation and changes requested were:

  • Change empty state and no results type
  • Only allow searching in list state

@khaykov Can you update the screenshots in the PR?

khaykov added some commits Jun 24, 2019

@khaykov

This comment has been minimized.

Copy link
Member Author

commented Jun 24, 2019

@SylvesterWilmott Updated the screenshots!

@malinajirka I think I addressed all of your comments :) Ready for another round!

@malinajirka

This comment has been minimized.

Copy link
Contributor

commented Jun 24, 2019

Both the feature and the code looks great now 😍, thanks!!

I've encountered two minor issues during the last test.

  • "card view" instead of the "compact" view is shown after configuration change (rotation)
  • a new request search request is dispatched after a configuration change (rotation)
    search

I think neither of the issues is critical. I'm approving this PR. Feel free to merge it and fix the issues in a different PR or fix them here. It's up to you. Thanks! 🙇

@malinajirka

This comment has been minimized.

Copy link
Contributor

commented Jun 25, 2019

Looks all good now;)! Thanks!

@malinajirka malinajirka merged commit 5657212 into develop Jun 25, 2019

4 checks passed

Peril :warning: Danger found some issues. Don't worry, everything is fixable.
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: strings-check Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details

@malinajirka malinajirka deleted the feature/simple_post_search branch Jun 25, 2019

@designsimply

This comment has been minimized.

Copy link
Contributor

commented Jul 3, 2019

Beta tested this using WPAndroid alpha-177 (12.8 alpha) on Pixel 3 Android 9 and it looks great! 👍

Screenshot_20190703-121941 Screenshot_20190703-121502 Screenshot_20190703-121400

I did expect searching for drafts to work (before I read the PR which notes that only searching for published/private posts work so far) and wanted to note there is a request already open for that at #9183. 😍

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