feat(settings): improve map pin ux

[oktomus]

PR Checklist

• - Commit messages are descriptive, it will be used in our Release Notes

PR Type

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Developer experience (improves developer workflows for contributing to the project)

Description

This is a first PR to improve the user experience when adding/editing/removing a map pin.
I decided to split the work in multiple PRs because I might have done things that don't match with the code guidelines, and the figma design suggested in #2999 collides with the current implementation.

What this PR does

• Lock editing of the pin behind a button, so that you don't accidentally edit your pin
• Add a map image preview when you don't have a pin
• Show the pin in read only when you open your profile (MapWithDraggablePin becomes MapBin with a draggable property)

What I want to do next

• Noticed saving sometimes fail with no error when editing the pin, I assume it's not related to this PR but needs testing
• The readonly preview of your pin will be like the Maps page of the website, showing a badge, description and image

@thisismattia, in the figma design, I see a Save map pin button, but it kinda collides with the UI flow of Save profile. I see two ways going forward:

• Keep the current flow, user has to press Save profile, no Save map pin button
• Add a save button on the map + add a confirmation popup for Remove pin
  Any thoughts?

Git Issues

Closes #2999

Screenshots/Videos

Here is a loom showcasing the new UX

---

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.

[codecov]

Codecov Report

Attention: Patch coverage is 27.10280% with 156 lines in your changes are missing coverage. Please review.

Project coverage is 62.57%. Comparing base (3ec12d6) to head (713bfce).
Report is 5 commits behind head on master.

❗ Current head 713bfce differs from pull request most recent head d8aec15. Consider uploading reports for the commit d8aec15 to get more accurate results

Files	Patch %	Lines
packages/components/src/MapWithPin/MapWithPin.tsx	18.18%	126 Missing ⚠️
packages/components/src/MapWithPin/MapPin.tsx	48.88%	23 Missing ⚠️
...ings/content/formSections/MemberMapPin.section.tsx	50.00%	7 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3094      +/-   ##
==========================================
- Coverage   65.85%   62.57%   -3.28%     
==========================================
  Files         411      373      -38     
  Lines       13303    11972    -1331     
  Branches     2431     2167     -264     
==========================================
- Hits         8761     7492    -1269     
+ Misses       4490     4429      -61     
+ Partials       52       51       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[cypress]

5 failed tests on run #5319 ↗︎

5

91

0

0

0

Details:

feat(settings): improve map pin ux
Project: onearmy-community-platform	Commit: c50d92bed7
Status: Failed	Duration: 08:13 💡
Started: Mar 20, 2024 7:29 PM	Ended: Mar 20, 2024 7:38 PM

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

View Output Video

Test		Artifacts
[Settings] > [Focus Workplace] > [Editing a new Profile]		Test Replay Screenshots Video
[Settings] > [Focus Member] > [Add a user pin]		Test Replay Screenshots Video
[Settings] > [Focus Machine Builder] > [Edit a new profile]		Test Replay Screenshots Video
[Settings] > [Focus Community Builder] > [Edit a new profile]		Test Replay Screenshots Video
[Settings] > Focus Plastic Collection Point > [Edit a new profile]		Test Replay Screenshots Video

Review all test suite changes for PR #3094 ↗︎

[benfurber]

This is a really like improvement to the app, thanks for picking it up @oktomus. A bunch of comments added and a couple more thoughts below.

I think it's worth pulling out DraggableMarker into it's own file with appropriate storybook/tests please.

There's a bunch more destructuring of useful objects about the place so things like this.state.hasMapPin don't have to be called all the time.

Please shout if you need me to clarify anything.

[oktomus]

Thanks for the review @benfurber! I addressed all the comments. For only two of them I didn't change anything but I commented.
I introduced a stories file for the MapPin but I'm not sure what is the purpose of it, tell me if I need to change things.

[github-actions]

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

https://onearmy-next--pr3094-feat-improve-map-pin-igaqwkbj.web.app

_{(expires Wed, 03 Jul 2024 04:42:06 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

_{Sign: 6d65e4f8fee2f6ab2da0c1c3b85b8797d66afa59}

[benfurber]

@thisismattia I'm happy with this from a code prospective, does it behave on preview as you'd expect?

[oktomus]

Were you guys able to have a look?

[davehakkens]

hey sorry for the extremely slow reply @oktomus and thanks for submitting such a complete PR. (with loom 🙏)

It works really nice. Behaviour like this makes sense and doesnt need to be aligned with the Figma so you can keep the flow as it is.

I do have one comment. It looks like you only implemented this for the member profiles not the other profile types
You can see them when you toggle here:

Could you implement it for the other profile types as well?

[oktomus]

Hey @davehakkens, sure I will have a look :)

[oktomus]

@davehakkens @benfurber Changes have been made, we now have only one component for the map pin section that is shared between members and workspace.

I noticed that the stars warning for workspace was always there, I left it as it but is that normal?

Sorry for the delay!

P.S: will there be a dev mission at Project Kamp next year? 🙏

[oktomus]

A bug I found with the map pin is that if you place your pin somewhere in the world where there is no street close-by, saving will fail because we don't have an address, but no warning/error message is being displayed. It's something I could take a look at in another PR.

Should I create an issue for it?

[oktomus]

@benfurber Do you know what can I do about the lock file being modified? It's in the CI failures

[benfurber]

@oktomus! Thank you so much for coming back to this after we basically blanked you for a bit!

I think the CI issue relates to problem we had a while back so I've merged in master and I think that'll fix it.

Yes please to raising the issue you've found.

Looks like there's a linting issue though? yarn format should sort that right out.

[benfurber]

Actually, let me run that unless you're free this very moment - I'd love to get this merged in.

[benfurber]

Annoyingly I thought we where there, but there's a cypress test failing! I tried to quickly add cy.get('[data-cy=add-a-map-pin]').click() in the right spot @oktomus. But there's more to it than that. Looks like there's some wierd state stuff going on with the form. I'll have to leave it with you.

[benfurber]

This happened to me on another form change. This might help: #3214

[oktomus]

Ok I will take a look :)

[oktomus]

@benfurber I took a look but I'm not sure how to fix it. Checkout this video for explanations: https://www.loom.com/share/a298aec7a7af4a2ebfeb68a3d3433a6c

Something is going off when calling setState

Do you think you can help me on that one? If you don't know either I can try to dig more

[pizzaisdavid]

Hope this isn't rude but I pulled the branch locally and tried to do some stuff. Much respect to @oktomus for all the work.

Summary

The code has changed since the last comment. Currently, there is a separate tab for the map, but switching between the tabs clears unsaved changes. And because there is no save button on the map tab, it is not possible to save map changes.

I'm not sure how would be best to precede: making the data sync between tabs, adding a save button to the map tab, or putting the map back on the original page.

I am unsure if the tab was introduced to resolve the state bug mentioned earlier in the thread or if it was a user design decision. I checked the Figma in the ticket and didn't see a separate tab there, but it could have been communicated somewhere else or maybe I didn't look in the right place. My guess is leaning towards that it was a work-around.

@oktomus if you would like to pair on this (either the state bug or making the un-saved data keep between tab changes) you can ping me. 😄

I suppose I could remove the commit making it a tab and investigate the bug myself. So also to clarify, @oktomus did you figure the state bug out? Did you introduce the tabs as a work-around?

Details

Before understanding the above, I fixed the merge conflicts and unit tests. Anyone who continues with this, feel free to take those changes.

The branch on my fork is called feat/oktomus-improve-map-pin-editor-ux, link. See the code comparison here.

I didn't fix the e2e tests because I am not sure which direction things will go implementation-wise, but there is a work-in-progress commit on my branch.

what I did step-by-step roughly, for people curious

Getting the branch on my local machine:

git remote add oktomus git@github.com:oktomus/community-platform.git
git fetch oktomus feat/improve-map-pin-editor-ux
git checkout feat/improve-map-pin-editor-ux

running on my localhost:

sudo sysctl fs.inotify.max_user_watches=131070
yarn
yarn start

fixing the merge conflicts:
(I already have the latest ONEARMY master because I forked ~10 minutes ago.)

git rebase master

There is a file, MapWithDraggablePin.tsx that was deleted on master but this merge request edits it.
To fix: delete it.

Then:

git rebase --continue

There is another merge conflict, in the test file src/pages/UserSettings/UserSettings.test.tsx

Relevant part on master:

<ThemeProvider theme={Theme}>
  <MemoryRouter
    initialEntries={[routerInitialEntry ? routerInitialEntry : '']}
  >
    <SettingsPage adminEditableUserId={isAdmin ? user._id : null} />
  </MemoryRouter>
</ThemeProvider>

Relevant part on this branch:

<ThemeProvider theme={Theme}>
  <MemoryRouter>
    <SettingsForm adminEditableUserId={isAdmin ? user._id : null} />
  </MemoryRouter>
</ThemeProvider>

There is a change in the things passed to MemoryRouter, but also there is a difference of SettingsForm and SettingsPage. To fix: change it to SettingsContainer as that was something introduced in this merge request to contain both tabs.
(I'd be in favor of just calling that SettingsPage but I am trying to make the minimal changes here.)

After making that change and updating the import. Running the unit tests (yarn test:unit), a single test fails:

UserSettings › map pin › displays moderation comments to user

Makes sense, we now need to click the map tab. (technically, there are two tests relating to the map pin but one passes anyways because it is checking something doesn't exist.)

The unit tests use userEvent to simulate user interaction. We need to click the map tab, add the following to both tests after the act block:

const mapButton = wrapper.getByText('Map')
await waitFor(() => mapButton.click())

Unit tests now pass, nice.

Time to check e2e tests (yarn test)

Some fail.

Add: data-cy="map-tab" to the <Tab> element.

Authors note: at this point I found the bug mentioned in the summary and wasn't sure how best to continue.

[oktomus]

Hi there, thanks for having a look @pizzaisdavid!

The tab is something we added together with @benfurber, but we haven't finished it and I didn't take the time to work on it since. Originally, there was no tab, and one test was failing.

Because a big redesign-refactor into tabs was planned for the profile page, we thought it was better to not fix the test and just start the redesign-refactor.

The best I think would be to have the map tab working. The tab is not a work-around, eventually there would be a tab for each section. I don't remember where but there is another place on the website where tabs are used, and the goal is to do the same in the profile page (you can probably find where by looking for Tab component usages).

However, maybe it's easier for you to fix the test instead of working on the tabs. In that case feel free to remove the commit and fix the test, maybe my last comments about it can give you some hints on what is wrong. But, you would be fixing something that would probably disappear some time soon.

Unfortunately I don't have time to sync at the moment, but I will try my best to follow this conversation and answer any question you might have!

[pizzaisdavid]

Thanks @oktomus for the information. It was helpful. 😃

Since a redesign is desired, then I agree, the best way forward is to continue with the tab.

@oktomus @benfurber Do you remember/know if there was a UI mock-up? My main question being if there should be a separate save button for the map pin. (I checked the Figma and didn't see anything for that https://www.figma.com/design/N1O9TzrKijPaZfYXqBboC8/Community-Platform)

@benfurber if there isn't anything, can I just make something up?

My proposal would be:

• Switching tabs doesn't clear the data between them.
• Each tab has its own save button for now.
• Ideally, when the user goes to close the tab, they'd get a warning if they have unsaved changes (unsure if like that now.)

Also, I don't promise that I'd get to this. 😜

[davehakkens]

FYI @dalibormrska is looking into the redesign of the profiles and tabs

[pizzaisdavid]

If there isn't an understanding yet of how the design should look/behave, then I would propose to remove the "awaiting author" label and add "Blocked" and/or "Under Discussion" labels.