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: wait for map load #1077

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

fix: wait for map load #1077

wants to merge 3 commits into from

Conversation

emilyjablonski
Copy link
Collaborator

@emilyjablonski emilyjablonski commented Jan 31, 2025

This PR addresses #1078

  • Addresses the issue in full
  • Addresses only certain aspects of the issue

Description

It is challenging to reproduce, but on a throttled connection, the listing list will sometimes flash an empty state before the full list has had a chance to actually do its first load.

How Can This Be Tested/Reviewed?

On both desktop and mobile, throttle your connection in the network tab. Load the page, pan the map, and add/remove filters. Ensure the listing list does not flash an empty state.

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

@@ -106,6 +117,8 @@ function ListingsSearchCombined(props: ListingsSearchCombinedProps) {
// Don't search the listings if the filter is changing - first search the markers, which will update the listings
if (
(!isFirstBoundsLoad &&
!!map &&
Copy link
Collaborator Author

@emilyjablonski emilyjablonski Jan 31, 2025

Choose a reason for hiding this comment

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

note: Only search for listings if the map has loaded (involves both new checks here) - this I cannot reproduce, but is just a suspicion on what may be happening

@@ -144,7 +157,9 @@ function ListingsSearchCombined(props: ListingsSearchCombinedProps) {
})

// On desktop, keep list loading set to true until map is finished with its first load
if (!isFirstBoundsLoad || !isDesktop) setIsLoading(false)
if ((!isFirstBoundsLoad && newMarkersSearch !== undefined) || !isDesktop) {
Copy link
Collaborator Author

@emilyjablonski emilyjablonski Jan 31, 2025

Choose a reason for hiding this comment

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

note: Only set the first page load loading state to false if the markers have been fetched for the first time, should prevent the flash

@@ -144,7 +157,9 @@ function ListingsSearchCombined(props: ListingsSearchCombinedProps) {
})

// On desktop, keep list loading set to true until map is finished with its first load
if (!isFirstBoundsLoad || !isDesktop) setIsLoading(false)
if ((!isFirstBoundsLoad && newMarkersSearch !== undefined && !changingFilter) || !isDesktop) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

note: Keep the loading state through fully changing the filter

@@ -233,6 +248,7 @@ function ListingsSearchCombined(props: ListingsSearchCombinedProps) {
}, [isFirstBoundsLoad])

const onFormSubmit = (params: ListingSearchParams) => {
setIsLoading(true)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

note: Initiate loading state earlier when the filter is changed

@@ -38,6 +38,7 @@ const MapRecenter = (props: MapRecenterProps) => {
)
}}
size={"sm"}
variant={"primary-outlined"}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

note: Change the recenter button to primary outline so that it doesn't blend in with the larger clusters

bathrooms={props.bathrooms}
counties={locations}
/>
<APIProvider apiKey={props.googleMapsApiKey}>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

note: Move API provider up so that the map check is accessible when calling the listings API

@@ -111,6 +111,7 @@

[class*="loading-overlay__spinner"] {
top: var(--seeds-s20);
color: var(--seeds-color-primary);
Copy link
Collaborator Author

@emilyjablonski emilyjablonski Jan 31, 2025

Choose a reason for hiding this comment

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

note: Change spinner color to primary - it was invisible on very first page load for a second on a slow connection before the rest of the list loaded

@@ -50,6 +51,9 @@ export const fitBounds = (
map.setZoom(zoomLevel - 7)
}
}
if (setIsFirstBoundsLoad) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

note: Move setting the bounds load to false into the set bounds function itself

@@ -203,11 +207,7 @@ export const MapClusterer = ({
// Only automatically size the map to fit all pins on first map load
if (isFirstBoundsLoad === false) return

fitBounds(map, mapMarkers)

setTimeout(() => {
Copy link
Collaborator Author

@emilyjablonski emilyjablonski Jan 31, 2025

Choose a reason for hiding this comment

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

note: Seems like this really shouldn't have been here, I think it was intended to be a setTimeout(0) - this was moved into the set bounds fxn

@@ -48,114 +47,110 @@ const ListingsCombined = (props: ListingsCombinedProps) => {

const getListingsList = () => {
return (
<APIProvider apiKey={props.googleMapsApiKey}>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

note: Just moved this up a level

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant