Formed in 2009, the Archive Team (not to be confused with the archive.org Archive-It Team) is a rogue archivist collective dedicated to saving copies of rapidly dying or deleted websites for the sake of history and digital heritage. The group is 100% composed of volunteers and interested parties, and has expanded into a large amount of related projects for saving online and digital history.
feat: clicking entity location searches for github users matching that location #802
Conversation
Add shared route for SearchUsersByLocations
Add componentDidMount to Search. Add setQueryAsLocation method to test params and search for users by location.
Remove tests referencing web communication to google maps now that clicking location takes user to search screen rather than out of the app.
Add location to the search bar in ios and android, to show text of the location
Feature/SeeUsersByLocation
Add contributors
|
Good teamwork! Left some comments for you. Feel free to tell me if you have different opinions. |
| @@ -206,6 +206,12 @@ const sharedRoutes = { | |||
| title: navigation.state.params.title, | |||
| }), | |||
| }, | |||
| SearchUsersByLocation: { | |||
chinesedfan
May 11, 2018
Member
In order to share this screen for other places, I suggest to define it as Search and replace the one in SearchStackNavigator with MainSearch.
| @@ -96,7 +97,7 @@ export const EntityInfo = ({ entity, orgs, locale, navigation }: Props) => { | |||
| type: 'octicon', | |||
| }} | |||
| subtitle={entity.location} | |||
| onPress={() => Communications.web(getLocationLink(entity.location))} | |||
| onPress={() => getUsersByLocation(entity.location, navigation)} | |||
chinesedfan
May 11, 2018
Member
Let's write the implementation here directly. getUsersByLocation sounds like returning users but it is not.
|
|
||
| if (params) { | ||
| this.switchQueryType(1); | ||
| this.search(`location:${params.location}`, 1); |
chinesedfan
May 11, 2018
Member
Please use SearchTypes.USERS, instead of 1. And you'd better check params && params.location.
|
|
||
| if (params) { | ||
| if (params.location) { | ||
| isLocation = `location:${params.location}`; |
chinesedfan
May 11, 2018
Member
isLocation usually means a boolean. How about naming it as searchText? And we can check in only one if statement, if (params && params.location).
| / /g, | ||
| '+' | ||
| )}` | ||
| ); |
chinesedfan
May 11, 2018
Member
Try to test your new press action. Mock navigation.navigate and check whether it is called with right arguments.
| @@ -2,4 +2,4 @@ distributionBase=GRADLE_USER_HOME | |||
| distributionPath=wrapper/dists | |||
| zipStoreBase=GRADLE_USER_HOME | |||
| zipStorePath=wrapper/dists | |||
| distributionUrl=https\://services.gradle.org/distributions/gradle-2.14.1-all.zip | |||
| distributionUrl=https\://services.gradle.org/distributions/gradle-4.3-rc-2-all.zip | |||
chinesedfan
May 11, 2018
Member
Is it really needed? If not, please do not commit, as well as several files like Cound/Failed/project.pbxproj.
By the way, if it didn't introduce too much changes, maybe you can upgrading the dependency. |
|
Hi @JSweet314! Thank you and other contributors for this PR Aside from what @chinesedfan already said, I left you a comment to further explain a little problem. Also, I'm noticing two new empty files "Could" and "Failed" which shouldn't have been included in this PR. (joys of bash scripting, heh) Don't hesitate to reach us on gitter if you need any help at all! |
| setQueryAsLocation = () => { | ||
| const { params } = this.props.navigation.state; | ||
|
|
||
| if (params) { |
machour
May 14, 2018
Member
We are already using navigation state in this screen to store/retrieve the searched keyword (see line 38 for example).
Here, it seems that as soon as there's a parameter, you assume that it's related to location, which seems wrong.
Screenshots
Description
Per the suggested solution, we changed the functionality of clicking on an entity's location to navigate to the search screen and initiate a search for users corresponding to the entity's location. This eliminates the need for the Communications.web method to direct the user outside of the application to google maps (deemed not very useful/interesting). Subsequently, we pass down the search query to the search screen from the navigation object's params to display the search term to the user in the RNSearchBar and MNSearchBar components. Rather than TabNavigator, an additional shared StackNavigator route was needed in order to allow for the passing of the params (upgrading the React-Navigation dependency to the current version would eliminate this need, however, that refactor would fall well outside the scope of this issue). You will note the new shared route titled 'SearchUsersByLocation' in the routes.js file. Navigating to this feature in this way also gives the user the added benefit of having a back arrow to return to the entity from which the location was derived.