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

fixed #981 #1285

Merged
merged 8 commits into from
Dec 26, 2024
Merged

fixed #981 #1285

merged 8 commits into from
Dec 26, 2024

Conversation

viditpawar0
Copy link
Contributor

@viditpawar0 viditpawar0 commented Dec 12, 2024

Please make sure these boxes are checked before submitting your pull request - thanks!

  • Apply the AndroidStyle.xml style template to your code in Android Studio.

  • Run the unit tests with gradlew connectedObaGoogleDebugAndroidTest to make sure you didn't break anything

  • If you have multiple commits please combine them into one commit by squashing them for the initial submission of the pull request. When addressing comments on a pull request, please push a new commit per comment when possible (reviewers will squash and merge using GitHub merge tool)

Fixed #981

Summary of changes: -

  • Implemented a mSelectAddressFromContactLauncher in TripPlanFragment that can be used to get the user select a contact and set the address from the contact to the text field.
  • Added a contact selector icon for both addresses in the fragment_trip_plan.xml.

Screenshots of UI Changes: -
Screenshot_20241212_210844_OneBusAway

@CLAassistant
Copy link

CLAassistant commented Dec 12, 2024

CLA assistant check
All committers have signed the CLA.

@viditpawar0
Copy link
Contributor Author

@aaronbrethorst I have signed the CLA, still it says that it is pending

@aaronbrethorst
Copy link
Member

@viditpawar0 I don't know why it says you haven't signed it, but I do see that you have completed it from the admin console. fyi @amrhossamdev

@amrhossamdev
Copy link
Member

Hi @viditpawar0

Thank you for your contribution. I will test that and get back to you!

@amrhossamdev amrhossamdev self-requested a review December 19, 2024 00:33
@amrhossamdev
Copy link
Member

Hi @viditpawar0, thank you for your contribution! I have tested the functionality, and it works perfectly. I will request code changes from you, and it will be ready for merging.

@viditpawar0
Copy link
Contributor Author

Thank you so much @amrhossamdev for taking time to review my code and provide insights. I have made the necessary changes. The reason I referred to the address text view in the mSelectAddressFromContactLauncher as a text view and not address input is because the mSelectAddressFromContactLauncher is able to update any TextView with the user selected contact's address. But you are right. Naming should be based on purpose and not ability.

@amrhossamdev
Copy link
Member

Great job, @viditpawar0! Your work is fantastic! I did notice a small bug: when I select an address for the "starting address" from my contacts, it works perfectly, and the address is set correctly. However, when I select an address for the "ending address," it removes the "starting address" from the input field. And vice versa. Could you please take a look at this? Let me know if you need any assistance!

@viditpawar0
Copy link
Contributor Author

Sure @amrhossamdev , that bug is solved in this commit.

@amrhossamdev
Copy link
Member

Well done, @viditpawar0. Another thing: you should request to submit to plan the trip after selecting the to address. It will be much better for user experience.

@viditpawar0
Copy link
Contributor Author

Okay, @amrhossamdev I will need a little assistance on that, do I need to call the checkRequestAndSubmit() method to submit to plan the trip?

@amrhossamdev
Copy link
Member

@viditpawar0 Yes, and we need to make sure that we have the from and to locations to submit a request.

@viditpawar0
Copy link
Contributor Author

@amrhossamdev I will need the api.geocode.earth API to test that which is not working on my device.

Here is the error:
image

Here is the http response:
{ "meta": { "version": 1, "status_code": 401 }, "results": { "error": { "type": "KeyError", "message": "No api_key specified." } } }

This is happening while using the Geocoder to get the list of addresses from the address string. It says that no api key is specified. Do I need to set up my own api key for that?

Please help me with this.

@amrhossamdev
Copy link
Member

@viditpawar0 Can you push your changes?

@viditpawar0
Copy link
Contributor Author

@amrhossamdev sure.

@viditpawar0
Copy link
Contributor Author

@amrhossamdev There you go. But note that this change is un-tested because the geocode api is not working on my device

@amrhossamdev
Copy link
Member

Thanks, @viditpawar0 I will test that!

@viditpawar0
Copy link
Contributor Author

Welcome, @amrhossamdev !

@amrhossamdev
Copy link
Member

Great work, @viditpawar0 ! 🥳 I made a couple of minor changes. When we select an address from the contacts, we will request focus to allow autocomplete suggestions for the address. This is essential to correctly obtain the latitude and longitude, which are necessary for the trip planning request. This ensures we have the precise location rather than just the address, and that's how trip planning works.

Additionally, I have fixed some minor bugs in the trip planning UI.

image

Overall, fantastic job, and thank you for your contribution! This pull request is ready to merge.

@viditpawar0
Copy link
Contributor Author

@amrhossamdev awesome🤩! I'm excited.

Signed-off-by: Amr Hossam <[email protected]>
@amrhossamdev
Copy link
Member

amrhossamdev commented Dec 26, 2024

Hi @viditpawar0, could you ensure you signed the CLA to merge?

I See you have two accounts 🤔 @arctik-circle and @viditpawar0 could you ensure both signed CLA ?

@viditpawar0
Copy link
Contributor Author

@amrhossamdev sure, done. Signed CLA from both accounts

@amrhossamdev
Copy link
Member

LGTM, well done 🫡

@amrhossamdev amrhossamdev merged commit 0da0ced into OneBusAway:main Dec 26, 2024
2 checks passed
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.

Trip planning - Allow planning from contact address
5 participants