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

Feature/navigation cancelled finished #235

Merged

Conversation

marcotrumpet
Copy link
Contributor

Referring to this and this

On iOS, event handling is fixed, now uses enums string instead of int so we can properly get on Dart side instead of relying on index.
When a user ends navigation we get navigation_finished events and also we can collect feedback (rating and comment) in Dart.

On Android, creating a custom InfoPanelEndNavButton (with the same UI) let us override the onTap action, I set it to stop the navigation and send back to dart the navigation_cancelled events.

@dschandrasekara
Copy link

@marcotrumpet Thanks a lot mate, @eopeter can we get new build with this fix soon ?
We already have deploy our iOS app to apple play store and but we didn't deploy our android app to play store duet to issue #229 , if you can prepare for the release , it is really good for us and this is really top urgent fix for us. We really appreciate your help. 👍

@eopeter
Copy link
Owner

eopeter commented May 22, 2023

@marcotrumpet Not entirely sure about this PR; Why does ending navigation depend on the Feedback?

Copy link
Owner

Choose a reason for hiding this comment

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

Why do we need to collect feedback from the user? Mapbox should be prompted to show its Feedback form or prevented from showing it if you do not wish to participate

Copy link
Owner

Choose a reason for hiding this comment

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

Why do we need to collect feedback from the user? Mapbox should be prompted to show its Feedback form or prevented from showing it if you do not wish to participate

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I prompt my user a route I want to know if everything was fine. It's not mandatory to use, if you don't care about user feedback you can simply ignore data in your app

Copy link
Owner

Choose a reason for hiding this comment

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

Yes, this seems to be too opinionated and should not be in the library. You can implement it in your Flutter app by hiding MapBox Feedback and providing your own feedback form. It should not be done in the plugin

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this seems to be too opinionated and should not be in the library. You can implement it in your Flutter app by hiding MapBox Feedback and providing your own feedback form. It should not be done in the plugin

I disagree. It's a mapbox feature so why is this not ok?
By the way your package your roule, feel free to reject this PR

Copy link
Owner

@eopeter eopeter May 23, 2023

Choose a reason for hiding this comment

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

It's not about my package my rule, it is a community package and we can have civilized discussions about it and come to an agreement. Without contributors, this package will not be where it is today.

I will merge this but I am requesting a change in the PR to make the override of the view optional. Users should opt in.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree with the civilized discussion. This can't happen though if you close issues pointing to PR with bad workaround and about different topic.

I would also suggest to open up the discussion section so we can discuss whether or not follow lint rules and best practices

Copy link
Owner

Choose a reason for hiding this comment

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

I opened up the discussions.

@marcotrumpet
Copy link
Contributor Author

@marcotrumpet Not entirely sure about this PR; Why does ending navigation depend on the Feedback?

It does not. It gives you more infos back (rating and comment from the native view) in case you need in flutter app. Main thing here is that now events are received properly on dart side. Previously there were an issue because the enum was index based but the ios enum are different than dart enum

@eopeter eopeter merged commit fc400d7 into eopeter:master May 24, 2023
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.

None yet

3 participants