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

feat: Add Places AutoComplete for Search #1306

Merged
merged 5 commits into from
Mar 24, 2019
Merged

feat: Add Places AutoComplete for Search #1306

merged 5 commits into from
Mar 24, 2019

Conversation

RotBolt
Copy link

@RotBolt RotBolt commented Mar 14, 2019

Fixes #799

Changes:

  1. Add MapBox Autocomplete Place View , as it doesn't need PlayServices
    So work on both F-droid and PlayStore version :)
    1.1) Changes done in the layout for Better UX
    1.2) Also shows Recents Location

  2. Change in "Current Location" and change in place of Current Location
    Because "Current Location button" could not come in between the AutoComplete Fragment

  3. Added a custom Round background used in "Current Location Button"
    For better and Nice UX

  4. Change in the color of ic_location_pin and used tint to change the color where needed

Screenshots for the change:

PlayStore

F-Droid

Update : Restore ActionBar

@RotBolt
Copy link
Author

RotBolt commented Mar 14, 2019

@iamareebjamal Please Review

@RotBolt RotBolt changed the title Feat: Add Places AutoComplete for Search feat: Add Places AutoComplete for Search Mar 14, 2019
@fossasia fossasia deleted a comment Mar 14, 2019
@fossasia fossasia deleted a comment Mar 14, 2019
when (requestCode) {
LOCATION_PERMISSION_REQUEST -> {
if (grantResults.isNotEmpty() && grantResults[0] == PackageManager.PERMISSION_GRANTED) {
geoLocationViewModel.configure()
} else {
Snackbar.make(rootView, R.string.cannot_fetch_location, Snackbar.LENGTH_SHORT).show()
Snackbar.make(rootView, R.string.cannot_fetch_location, Snackbar.LENGTH_SHORT)
.show()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary change

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just ran Code Format , it did that automatically.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't run auto format in PRs.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do I have to revert this change ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok will do it

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ShridharGoel I have removed unnecessary changes . Please merge this

@iamareebjamal
Copy link
Member

Great work. Ask your peers to review it first

@RotBolt
Copy link
Author

RotBolt commented Mar 16, 2019

Great work. Ask your peers to review it first

Thank you. And Would take care of this thing first

Signed-off-by: thelimitbreaker <rohanmaityofficial@gmail.com>
Copy link
Member

@liveHarshit liveHarshit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action bar is missing in search fragment.

inflater: LayoutInflater,
container: ViewGroup?,
savedInstanceState: Bundle?
): View? {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary changes.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Umm . This is for better view of parameters used. Still don't want . I would revert it

@liveHarshit
Copy link
Member

A search bar should be there. Why you remove it?
Please check search in Eventbrite, how it works.

@RotBolt
Copy link
Author

RotBolt commented Mar 20, 2019

A search bar should be there. Why you remove it?
Please check search in Eventbrite, how it works.

@liveHarshit there is search bar in the fragment itself . You could see in the gif .

@RotBolt
Copy link
Author

RotBolt commented Mar 20, 2019

Action bar is missing in search fragment.

@liveHarshit , Autocomplete fragment itself give a back button . Having action bar for back button and fragment also showing backbutton will create UI confusion.

@RotBolt
Copy link
Author

RotBolt commented Mar 20, 2019

@liveHarshit eventbrite uses Google's Autocomplete which would require Play services and show the results in custom layout. I could do that . But it won't work for f-droid (because of Play services) .
Current MapBox Autocomplete works on both and provides same UI for both versions

@liveHarshit
Copy link
Member

Action bar is missing in search fragment.

@liveHarshit , Autocomplete fragment itself give a back button . Having action bar for back button and fragment also showing backbutton will create UI confusion.

I'm asking about the action bar in the search fragment, not in search location fragment.

@RotBolt
Copy link
Author

RotBolt commented Mar 20, 2019

I'm asking about the action bar in the search fragment, not in search location fragment.

I have done no changes in SearchFragment.kt . You could see in Files Changed

@liveHarshit
Copy link
Member

liveHarshit commented Mar 20, 2019

I'm asking about the action bar in the search fragment, not in search location fragment.

I have done no changes in SearchFragment.kt . You could see in Files Changed

One change reflects another.

thisActivity.supportActionBar?.show()
thisActivity.supportActionBar?.title = ""
thisActivity.supportActionBar?.setDisplayHomeAsUpEnabled(true)
thisActivity.supportActionBar?.hide()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here you hide it in activity, not in the fragment, so you need to show it again in search fragment.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm , that for removing actionbar in SearchLocationFragment.kt.
Will have to enable the actionbar in onResume() of SearchFragment.kt
Thanks for pointing it out :) .will do it

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not onReasume, show it in onDestroyView of search location fragment.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah sorry . I forgot these are fragments. Will have to call this in onDestroyView .

private fun checkLocationPermission() {
val permission =
ContextCompat.checkSelfPermission(requireContext(), Manifest.permission.ACCESS_COARSE_LOCATION)
ContextCompat.checkSelfPermission(requireContext(),
Manifest.permission.ACCESS_COARSE_LOCATION)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary changes.

@@ -91,7 +81,42 @@ class SearchLocationFragment : Fragment() {
Navigation.findNavController(rootView).popBackStack(fragmentId, false)
}

override fun onRequestPermissionsResult(requestCode: Int, permissions: Array<String>, grantResults: IntArray) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also change override method in MainActivity.

@RotBolt
Copy link
Author

RotBolt commented Mar 21, 2019

@liveHarshit I have updated it Removed unecessary changs and actionbar is also shown

Copy link
Member

@liveHarshit liveHarshit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please update the GIF as well?

@@ -31,22 +31,19 @@ class SearchLocationFragment : Fragment() {
private val geoLocationViewModel by viewModel<GeoLocationViewModel>()
private val safeArgs: SearchLocationFragmentArgs by navArgs()

private val AUTOCOMPLETE_FRAG_TAG = "AutoComplete_Frag"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Declare it as constant.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@liveHarshit
Copy link
Member

Also change override method in MainActivity.

And what about it?

@RotBolt
Copy link
Author

RotBolt commented Mar 21, 2019

Also change override method in MainActivity.

And what about it?

I didn't change Override method then, It was syntax change, that was show,n back then . You could see in files changed

@liveHarshit
Copy link
Member

Earlier you have removed onRequestPermissionsResult. It is calling from MainActivity, so I asked to change it. Now it is okay.

@liveHarshit
Copy link
Member

The current location button at the bottom looks odd. Can you please add it at top of the list like google map.

@RotBolt
Copy link
Author

RotBolt commented Mar 21, 2019

Change in "Current Location" and change in place of Current Location
Because "Current Location button" could not come in between the AutoComplete Fragment

I mentioned this, It could not come in between, search bar and list.

Alternate way is

  • Use Google Autocomplete API,
  • get result (JSON)
  • Show it in custom view,
    but that also would support only PlayStore only (Due to Play services issue)

F-Droid would still be left

@liveHarshit
Copy link
Member

liveHarshit commented Mar 21, 2019

@iamareebjamal Your opinion?

@iamareebjamal
Copy link
Member

Seems fine if it is not our UI. It's less code to maintain. @liveHarshit Please approve if everything else is fine

liveHarshit
liveHarshit previously approved these changes Mar 21, 2019
Copy link
Member

@liveHarshit liveHarshit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

iamareebjamal
iamareebjamal previously approved these changes Mar 21, 2019
nikit19
nikit19 previously approved these changes Mar 23, 2019
@iamareebjamal
Copy link
Member

Resolve conflicts

@RotBolt RotBolt dismissed stale reviews from nikit19, iamareebjamal, and liveHarshit via 0010cf5 March 24, 2019 18:37
iamareebjamal
iamareebjamal previously approved these changes Mar 24, 2019
@RotBolt
Copy link
Author

RotBolt commented Mar 24, 2019

@iamareebjamal , after merging latest commits. New Voice button is added . But it was added for old layout. Now for adding autoplace frag. I might again have to change the layout . Could I do that ?

@iamareebjamal iamareebjamal merged commit c22db3a into fossasia:development Mar 24, 2019
@RotBolt RotBolt deleted the feature/autocomplete-place branch April 5, 2019 02:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants