refactor: migrate search screen to the new API #758

machour · 2018-03-30T11:19:15Z

Question	Response
Version?	v1.4.1
Devices tested?	iPhone 7
Bug fix?	no
New feature?	yes
Includes tests?	no
All Tests pass?	yes
Related ticket?	#751

Description

Refactored the search screen to make use of the new API. Search results are now paginated & cached!

coveralls · 2018-03-30T11:26:48Z

Coverage decreased (-0.5%) to 42.935% when pulling 458a538 on machour:refactor/search-screen into d77aa82 on gitpoint:master.

machour

Left some comments to explain normalizrKey

machour · 2018-03-30T11:24:35Z

src/api/client.js

+        response,
+        nextPageUrl: this.getNextPageUrl(response),
+        schema: Schemas.REPO_ARRAY,
+        normalizrKey: 'items',


This is the first use of normalizrKey.

The API response for this call looks like the following:

{ total_count: 645354, incomplete_results: false, items: [ { /* first repo object */}, { /* second repo object */}, ... ] }

We only care for what's inside items, the array of repositories.

normalizrKey tells the dispatch proxy to only treat that property of the JSON, instead of all the JSON.

NICE ⭐️

machour · 2018-03-30T11:25:03Z

src/search/screens/search.screen.js

+const SearchTypes = {
+  REPOS: 0,
+  USERS: 1,
+};


introduced this little enum to make the code more readable

machour · 2018-03-30T11:26:12Z

src/search/screens/search.screen.js

+
+  const reposSearchResultsPagination = SEARCH_REPOS[searchQuery] || {
+    ids: [],
+    isFetching: false,


contrarily to other PRs, when we're loading this screen, we're not fetching yet, so we set isFetching to false.

machour · 2018-03-30T11:27:04Z

src/search/screens/search.screen.js

@@ -127,19 +161,22 @@ class Search extends Component {
    const selectedSearchType =
      selectedType !== null ? selectedType : this.state.searchType;

-    if (query !== '') {
+    if (query.trim() !== '') {


Added a trim() here. Currently, if you search for " ", the app will crash!

machour · 2018-03-30T11:27:52Z

src/search/screens/search.screen.js

@@ -127,19 +161,22 @@ class Search extends Component {
    const selectedSearchType =
      selectedType !== null ? selectedType : this.state.searchType;

-    if (query !== '') {
+    if (query.trim() !== '') {
+      const searchedQuery = query.toLowerCase();


GitHub search is case insensitive. We lowercase the query string in order to avoid having two paginations for nothing (Gitpoint vs gitpoint)

machour · 2018-03-30T11:28:45Z

src/search/screens/search.screen.js

      });
-      if (selectedSearchType === 0) {
-        searchRepos(query);
+      this.props.navigation.setParams({ [NAV_QUERY_PARAM]: searchedQuery });


We save the query string in the navigation, to be able to get it in MapStateToProps and select the corresponding pagination

machour · 2018-03-30T11:30:14Z

src/search/screens/search.screen.js

+      ].nextPageUrl === null
+    ) {
+      return null;
+    }


Here, we conditionally select the good pagination.

I can brake the code to have something less barbaric. This will change anyways when we introduce Code & Wikis search

Just wondering, how are you thinking we can simplify this when code & wikis are introduced?

Also, in the meantime, maybe we can simplify the following to something like:

renderFooter = () => { const { searchType } = this.state; const { reposSearchResultsPagination, usersSearchResultsPagination } = this.props; const currResultsPagination = searchType === SearchTypes.REPOS ? reposSearchResultsPagination : usersSearchResultsPagination; if (currResultsPagination.nextPageUrl) { return ( <View style={{ paddingVertical: 20, }} > <ActivityIndicator animating size="large" /> </View> ); } };

And another thought (if not know) - maybe we can extract this to a separate component (<LoadingFooter />?) if this logic is duplicated exactly the same in multiple screens?

I was thinking about having getActiveResultsPagination() & getActiveResults() instead of using ifs everywhere.

And for the refactoring, I was thinking of:

a <PaginatedFlatList> which would take a pagination prop and do everything for us.

a getPagination() in utils to avoid this chunk of code in mapStateToProps:

const reposSearchResultsPagination = SEARCH_REPOS[searchQuery] || { ids: [], isFetching: false, }

was just holding off on changing stuff until I got a few PRs merged. I think I'll do that next

machour · 2018-03-30T11:30:54Z

src/search/screens/search.screen.js

+                      ? 'searchRepos'
+                      : 'searchUsers'
+                  ](query, { loadMore: true })
+                }


same remark as above.

machour · 2018-03-30T11:31:27Z

root.reducer.js

@@ -4,7 +4,6 @@ import { userReducer } from 'user';
 import { repositoryReducer } from 'repository';
 import { organizationReducer } from 'organization';
 import { issueReducer } from 'issue';
-import { searchReducer } from 'search';


pew pew pew 🔫

housseindjirdeh

Aside from one tiny thing, great stuff fam.

I'm excited, you're improving every screen in this app and simplifying our entire Redux store in the same time <3

housseindjirdeh · 2018-03-31T00:25:27Z

src/search/screens/search.screen.js

+      ].nextPageUrl === null
+    ) {
+      return null;
+    }


Just wondering, how are you thinking we can simplify this when code & wikis are introduced?

Also, in the meantime, maybe we can simplify the following to something like:

renderFooter = () => { const { searchType } = this.state; const { reposSearchResultsPagination, usersSearchResultsPagination } = this.props; const currResultsPagination = searchType === SearchTypes.REPOS ? reposSearchResultsPagination : usersSearchResultsPagination; if (currResultsPagination.nextPageUrl) { return ( <View style={{ paddingVertical: 20, }} > <ActivityIndicator animating size="large" /> </View> ); } };

And another thought (if not know) - maybe we can extract this to a separate component (<LoadingFooter />?) if this logic is duplicated exactly the same in multiple screens?

machour · 2018-03-31T12:43:07Z

@housseindjirdeh refactoring done, now our render() method doesn't check for the search type anymore (aside for a translation) 🙌

Adding a new search type should be as simple as declaring it in the enum and the switch/case in the new methods.

I also spotted and fixed a minor UI problem, where the Loading list footer was displayed during the first search, resulting in two Loaders being displayed.

machour · 2018-03-31T12:47:24Z

By the way, noticed a bad UX design. Having a single FlatList to handle the various search type results may not be a good idea:

If you search for "react", display results for Repositories and Users, then scroll down on Repositories, when you get back to the Users tab, the list will have been scrolled.

Something to address in another PR.

housseindjirdeh

So so great, thank you so much 🚀

housseindjirdeh · 2018-04-02T00:48:34Z

And aaah I've never caught that before :( That really isn't great behaviour - opened #764 for this

machour added 4 commits March 30, 2018 11:59

refactor: migrate search screen to the new API

7a5fe4c

refactor: introduce an enum for search types

98c0e58

refactor: use a more friendly name

f7f3d9b

test: remove obosolete test

efd82a4

machour added refactor ready for review labels Mar 30, 2018

machour commented Mar 30, 2018

View reviewed changes

machour mentioned this pull request Mar 30, 2018

Rest: Migrate paginated calls to the new API #751

Open

13 tasks

housseindjirdeh reviewed Mar 31, 2018

View reviewed changes

machour added 4 commits March 31, 2018 10:25

Merge branch 'master' into refactor/search-screen

f822ac2

refactor: start paving the way for other search types

8536ab5

refactor: start paving the way for other search types

3d93468

fix: don't show listfooter if this is the initial search

458a538

machour mentioned this pull request Mar 31, 2018

Expand search to include issues #12

Open

housseindjirdeh approved these changes Apr 2, 2018

View reviewed changes

housseindjirdeh merged commit 3991e82 into gitpoint:master Apr 2, 2018

machour deleted the refactor/search-screen branch April 2, 2018 03:25

refactor: migrate search screen to the new API #758

refactor: migrate search screen to the new API #758

Uh oh!

Conversation

machour commented Mar 30, 2018

Description

Uh oh!

coveralls commented Mar 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

machour left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

housseindjirdeh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

machour commented Mar 31, 2018

Uh oh!

machour commented Mar 31, 2018

Uh oh!

housseindjirdeh left a comment

Choose a reason for hiding this comment

Uh oh!

housseindjirdeh commented Apr 2, 2018

Uh oh!

Uh oh!

coveralls commented Mar 30, 2018 •

edited

Loading