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

fix: map mobile recenter button #1065

Merged
merged 3 commits into from
Jan 27, 2025
Merged

Conversation

emilyjablonski
Copy link
Collaborator

@emilyjablonski emilyjablonski commented Jan 27, 2025

This PR addresses

Related to: #1044

Description

Because we moved a map API call into the list view, we were getting map rendering issues on mobile, because the list didn't have doesn't have direct access to the map like it does on desktop.

How Can This Be Tested/Reviewed?

Ensure that on mobile, the list renders, and the "recenter map" button never appears in the list view, regardless of the filters applied. Will want to test this on a real mobile device once it's deployed in staging, but looks good locally on a mobile device as well.

Author Checklist:

  • Added QA notes to the issue with applicable URLs
  • Reviewed in a desktop view
  • Reviewed in a mobile view
  • Reviewed considering accessibility
  • Added tests covering the changes
  • Made corresponding changes to the documentation
  • Ran yarn generate:client and/or created a migration when required

Review Process:

  • Read and understand the issue
  • Ensure the author has added QA notes
  • Review the code itself from a style point of view
  • Pull the changes down locally and test that the acceptance criteria is met
  • Either (1) explicitly ask a clarifying question, (2) request changes, or (3) approve the PR, even if there are very small remaining changes, if you don't need to re-review after the updates

@emilyjablonski emilyjablonski marked this pull request as ready for review January 27, 2025 00:36
Copy link
Collaborator

@ludtkemorgan ludtkemorgan left a comment

Choose a reason for hiding this comment

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

This change work. Thanks for the quick fix.

Two questions:

  • I'm wondering if we could also take a pass at this down the road so that in mobile list view the "mapMarker" api call is not even done. The fewer API calls we do the better the performance will be. I know that is a bigger change though so let's not do it as a part of this
  • The highlight of the "Map View" and "List View" buttons stays highlighted longer than expected. I can click around in the map and the button that says "List View" stays highlighted. This confused me when I first saw it because I thought it was telling me I was on "List" view instead of the map view. Is this intentional?
    image

@ludtkemorgan ludtkemorgan added ready to merge Should be applied when a PR has been reviewed and approved and removed 1 review needed labels Jan 27, 2025
@emilyjablonski
Copy link
Collaborator Author

@ludtkemorgan I added an explicit mobile check! I will say, I don't think this tackles your specific ask, which (to confirm) sounds like not making the map marker call on mobile until a user visits the map version of the page?

And for the button issue, I think I've also seen that a few times! I haven't been able to consistently reproduce it. I think it might be an issue with the focus state on the Seeds button. I'll see if I can repro it consistently with new steps.

@ludtkemorgan
Copy link
Collaborator

@emilyjablonski the additional changes look good!

@emilyjablonski emilyjablonski merged commit 55c5824 into main Jan 27, 2025
16 checks passed
@emilyjablonski emilyjablonski deleted the map-mobile-recenter-button branch January 27, 2025 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Should be applied when a PR has been reviewed and approved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants