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

Update map setting display #3862

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

LahuenGR
Copy link

@LahuenGR LahuenGR commented Sep 25, 2024

PR Checklist

PR Type

What kind of change does this PR introduce?

  • Bugfix (fixes an issue)
  • Feature (adds functionality)
  • Code style update
  • Refactoring (no functional changes)
  • Build related changes
  • CI related changes
  • Documentation changes
  • Other... Please describe:

What is the current behavior?

Standard pin showing in the Map Settings and without relevant information

2024-09-25 04_16_23-Window

What is the new behavior?

  1. Pin is now showing the profile type design and, when clicked, shows the popup with relevant information
  2. Removed one-click option to move the pin and setted it to double-click, as it was not working well with the click-n-popup idea

2024-09-25 04_20_16-Window

Describe the new behaviour
If useful, provide screenshot or capture to highlight main changes

Does this PR introduce a breaking change?

  • Yes
  • No

Git Issues

Closes #3856

What happens next?

Thanks for the contribution! We try to make sure all PRs are reviewed ahead of our monthly maintainers call (first Monday of the month)

If the PR is working as intended it'll be merged and included in the next platform release, if not changes will be requested and re-reviewed once updated.

If you need more immediate feedback you can try reaching out on Discord in the Community Platform development channel.

Copy link

cypress bot commented Sep 25, 2024

onearmy-community-platform    Run #6341

Run Properties:  status check failed Failed #6341  •  git commit 4853c55064: feat(component): update map settings display
Project onearmy-community-platform
Run status status check failed Failed #6341
Run duration 05m 29s
Commit git commit 4853c55064: feat(component): update map settings display
Committer Lahuen Garcia
View all properties for this run ↗︎

Test results
Tests that failed  Failures 2
Tests that were flaky  Flaky 2
Tests that did not run due to a developer annotating a test with .skip  Pending 0
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 73

Tests for review

Failed  src/integration/settings.spec.ts • 2 failed tests • ci-chrome

View Output Video

Test Artifacts
[Settings] > [Focus Member] > [Edit a new profile] Test Replay Screenshots Video
[Settings] > [Focus Machine Builder] > [Edit a new profile] Test Replay Screenshots Video
Flakiness  src/integration/howto/write.spec.ts • 1 flaky test • ci-chrome

View Output Video

Test Artifacts
[How To] > [Create a how-to] > [Warning on leaving page] Test Replay Screenshots Video
Flakiness  src/integration/research/write.spec.ts • 1 flaky test • ci-chrome

View Output Video

Test Artifacts
[Research] > [Displays draft updates for Author] > [By Authenticated] Test Replay Screenshots Video

Copy link
Member

@benfurber benfurber left a comment

Choose a reason for hiding this comment

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

Thanks @LahuenGR. I've added on quick style thing I could see but it's a little hard to review fully as you're pulling in unrelated commits from your other branch. Plus maybe some unrelated model changes?

Also. Have you looked into our cypress tests yet. Any ideas on how to test your change within the existing settings test(s)?

packages/components/src/MapWithPin/MapWithPin.stories.tsx Outdated Show resolved Hide resolved
@benfurber benfurber added the Review allow-preview ✅ Has received manual check for malicious code and can be safely built for preview label Sep 25, 2024
Copy link
Contributor

github-actions bot commented Sep 25, 2024

Visit the preview URL for this PR (updated for commit 7ca811e):

https://onearmy-next--pr3862-update-map-setting-d-uvghrep1.web.app

(expires Sat, 26 Oct 2024 21:06:39 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: 6d65e4f8fee2f6ab2da0c1c3b85b8797d66afa59

@LahuenGR
Copy link
Author

LahuenGR commented Sep 25, 2024

@benfurber I'm not sure why it dragged the commit from the other branch 🤔 let me try to fix it

What model changes are you referring to?

As for the cypress tool... nope, never used it hahaha, i'm going to need some help with that <3

Edit: I exported ILatLang type and removed one unused declaration, the last one i'm not sure why but seemed unnecessary there lol, could put it back there tho
2024-09-25 06_22_43-Window

@benfurber
Copy link
Member

From QA.

  1. Definitely need to increase the map height. If you change your profile type to something other than a member you'll see what I mean.
  2. Add onblur to hide the pin when click/tapping the map. This is especially important for mobile.
  3. Move the +/- controls to the bottom left, on mobile they nearly overlap with the search for address.
  4. For pin onhover change the cursor to grab.
  5. I don't think the explainer text is right because it is possible still to grab to move your pin. Let's go with: "To move your pin, grab it to move it or double click where you want it to go. Tap on your pin to see how it'll look on the map.

@LahuenGR
Copy link
Author

LahuenGR commented Sep 25, 2024

Did i understand correctly? @benfurber

Demo.mp4

packages/components/src/MapWithPin/MapPin.tsx Outdated Show resolved Hide resolved
packages/components/src/MapWithPin/MapPin.tsx Outdated Show resolved Hide resolved
src/models/common.models.tsx Outdated Show resolved Hide resolved
src/pages/UserSettings/SettingsPageMapPin.tsx Outdated Show resolved Hide resolved
src/models/index.ts Outdated Show resolved Hide resolved
@benfurber benfurber added the 🤝 Awaiting author Waiting on action from the author label Sep 26, 2024
@LahuenGR LahuenGR force-pushed the update-map-setting-display branch 8 times, most recently from 35c5286 to fccb66f Compare September 26, 2024 20:44
Pin is now like the Map page and, when clicked, displays the popup

fix ONEARMY#3856
Pin is now like the Map page and, when clicked, displays the popup

fix ONEARMY#3856
@benfurber
Copy link
Member

Hey @LahuenGR I've sorted out the complex rebase that was needed on this branch after we merged in a big refactor.

I've run the branch locally and I think there's a couple more bits to finish. If you won't have time to do it, no worries, just let me know and I can finish it off.

Screenshot 2024-10-08 at 15 03 37
  1. Let's move those controls (+/- and location search) to the top.
  2. For some reason the zoom out button doesn't work. Not sure if that relates to your branch or not but if you could take a look that would be awesome.

You're using ondragstart and ondragend for props with the map. On MapWithList, etc. we use the onclick prop to close a pin. Might be worth trying that?

@LahuenGR
Copy link
Author

LahuenGR commented Oct 8, 2024

Hey @LahuenGR I've sorted out the complex rebase that was needed on this branch after we merged in a big refactor.

I've run the branch locally and I think there's a couple more bits to finish. If you won't have time to do it, no worries, just let me know and I can finish it off.

Screenshot 2024-10-08 at 15 03 37 1. Let's move those controls (+/- and location search) to the top. 2. For some reason the zoom out button doesn't work. Not sure if that relates to your branch or not but if you could take a look that would be awesome.

You're using ondragstart and ondragend for props with the map. On MapWithList, etc. we use the onclick prop to close a pin. Might be worth trying that?

Hello sr!, i was waiting for your review hahaha

Nice, i'll be trying to solve those things <3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🤝 Awaiting author Waiting on action from the author Mod: Maps 🗺 Review allow-preview ✅ Has received manual check for malicious code and can be safely built for preview
Projects
Status: No status
Status: 💬 Changes Requested/with author
Development

Successfully merging this pull request may close these issues.

feat: update map setting display
2 participants