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/remove You-have-arrived-step #461

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

matbmapspeople
Copy link
Contributor

@matbmapspeople matbmapspeople commented Feb 6, 2025

Overview:
It was decided to remove last step from the directions (You have arrived). This steps is not something that Web SDK is serving.

Technical overview:

  • In the components, I removed code responsible for rendering You have arrived last step.
  • I performed scouting - resolving eslint warning.
  • In the Map Template project, I removed code responsible for defining last step - in this case You have arrived step. I removed logic that centers view port on destination location - we want to see last route leg in the view port.

Working solution:

Screen.Recording.2025-02-06.at.12.53.49.mov

@matbmapspeople matbmapspeople self-assigned this Feb 6, 2025
@matbmapspeople matbmapspeople marked this pull request as draft February 6, 2025 11:32
@matbmapspeople matbmapspeople marked this pull request as ready for review February 6, 2025 11:55
@andreeaceachir142
Copy link
Contributor

@matbmapspeople in the Directions.jsx file we have this function which seems to have been removed from your code.
Make sure to delete the function and all the instances if not needed anymore.

image image

@andreeaceachir142
Copy link
Contributor

@matbmapspeople this should be a minor, not a patch. It's not a feature addition, but it's a feature change. It's not really a bug that is being fixed. 🙏

@matbmapspeople matbmapspeople added minor and removed patch labels Feb 6, 2025
@andreeaceachir142
Copy link
Contributor

@matbmapspeople also remember to update the changelog for the components before merging this into main.
From what I can see, the changelog is behind one version. Please check with the team on what is missing so we can add the correct version of the changelog.

image

vs

https://www.npmjs.com/package/@mapsindoors/components

@andreeaceachir142
Copy link
Contributor

@matbmapspeople is the code here still needed, as it is for the destination step?
Uploading image.png…

@matbmapspeople
Copy link
Contributor Author

Im not able to see the image ☝️

Screenshot 2025-02-07 at 07 41 12

@matbmapspeople
Copy link
Contributor Author

@matbmapspeople also remember to update the changelog for the components before merging this into main. From what I can see, the changelog is behind one version. Please check with the team on what is missing so we can add the correct version of the changelog.

image vs

https://www.npmjs.com/package/@mapsindoors/components

Done

@andreeaceachir142
Copy link
Contributor

Hmm it might be this one @matbmapspeople
image

@matbmapspeople
Copy link
Contributor Author

matbmapspeople commented Feb 7, 2025

Yes, still need that code ✅ @andreeaceachir142
We need to distinguish when to show Cancel and when Finish Route:

Screenshot 2025-02-07 at 09 11 16

@matbmapspeople
Copy link
Contributor Author

Do you have any more comments @andreeaceachir142 on this merge request?

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.

2 participants