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

Adds Button To Change Map Style #1054

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

Conversation

DonovanAndrews300
Copy link

@DonovanAndrews300 DonovanAndrews300 commented Dec 14, 2024

Description

This PR introduces a button in the top-left corner of the Find Properties map. The button allows users to toggle between the three currently enabled map styles. Addresses (Issue#890)


Steps to Test

  1. Navigate to the Find Properties tab.
  2. Verify that the map switcher component appears in the top-left corner of the map.
  3. Try to apply each map style
  4. Ensure styles are correctly being applied to the map component
  5. Ensure the property list loads as expected.
  6. Confirm the map functionality remains consistent with previous behavior.

Screenshots

Screencast.from.12-17-2024.02.38.09.PM.webm

Copy link

vercel bot commented Dec 14, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
vacant-lots-proj ❌ Failed (Inspect) Dec 17, 2024 7:30pm

@@ -166,16 +164,7 @@ const PropertyDetailSection: FC<PropertyDetailSectionProps> = ({
return featuresInView.slice(start, end);
}, [page, featuresInView, smallScreenMode]);

return hasLoadingError ? (
Copy link
Author

Choose a reason for hiding this comment

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

The error UI was removed because changing the map style temporarily causes an error due to no items being in the property list. I noticed there is a separate UI designed specifically for "no results," rather than displaying the error UI. I wanted to bring this to attention.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@DonovanAndrews300 before removing the error UI, can you look at PR #732? The error UI appears if the map fails to fetch data. Removing that UI might have unexpected consequences and re-introduce a bug that was fixed by that previous PR.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for this you were absolutely correct in removing the error UI causing breaking changes for the reasons mentioned in the PR you linked. I added the UI back in and instead just had it skip setting the error state in the map component depending on the error received.

@CodeWritingCow CodeWritingCow changed the base branch from main to staging December 15, 2024 20:36
Copy link
Collaborator

@CodeWritingCow CodeWritingCow left a comment

Choose a reason for hiding this comment

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

@DonovanAndrews300 can you use the switch control mentioned in the ticket requirements? Thanks!

@nlebovits
Copy link
Collaborator

@DonovanAndrews300 just wanted to chime in and say thanks for working on this! Progress looks good--excited to see it deployed.

@nlebovits
Copy link
Collaborator

@CodeWritingCow is this ready to go? Can you merge it if it is?

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.

3 participants