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

Fully implement lazy loading #142

Merged
merged 19 commits into from
Feb 4, 2024
Merged

Fully implement lazy loading #142

merged 19 commits into from
Feb 4, 2024

Conversation

tunglinn
Copy link
Collaborator

@tunglinn tunglinn commented Nov 20, 2023

Parking lot data is now split by city, to allow for lazy loading.
Parking lots will only be loaded if the city is within view.
Added testing.

tunglinn and others added 6 commits November 20, 2023 10:48
Next step is to get the map to render using the split files.

Co-authored-by: tunglinn <[email protected]>
Mostly to get the autoscord card update, to help with lazy loading.

---------

Co-authored-by: Eric Arellano <[email protected]>
Co-authored-by: tunglinn <[email protected]>
Right now, parking lots will only load if you use the dropdown. However,
a better implementation is to "Load when dragged into view". Might wait
for #143 to implement.
To-do:
- [x] Load when dragged into view
- [x] Tests

---------

Co-authored-by: tunglinn <[email protected]>
Add tests

---------

Co-authored-by: tunglinn <[email protected]>
@tunglinn tunglinn marked this pull request as ready for review December 22, 2023 03:51
Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

This looks great! Thank you for the well written code, including the error messages in base.js.

Comment on lines 172 to 174
if (loadedCity.feature.properties.id === cityId) {
load = false;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't follow this. Instead, should the logic be

const city = parkingLayer.getLayers().find((city) => city.feature.properties.id == cityId);
load = city.feature.properties.<something>

Right now, this seems like it will always reload the same city no matter what, even if it was loaded before.

Copy link
Collaborator Author

@tunglinn tunglinn Jan 10, 2024

Choose a reason for hiding this comment

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

parkingLayer is the layer of already loaded lots.
So the logic right now is, if the city we want to load is already loaded (aka in parkingLayer), then don't load it.

src/js/setUpSite.js Show resolved Hide resolved
src/js/setUpSite.js Show resolved Hide resolved
@@ -246,3 +246,33 @@ test("scorecard pulls up city closest to center", async ({ page }) => {
await expect(scoreCardTitle).toEqual("Birmingham, AL");
await expect(cityToggleValue).toEqual("birmingham-al");
});

test("map only loads parking lots for visible cities", async ({ page }) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Great job testing this!

tests/scripts/base.test.js Show resolved Hide resolved
scripts/base.js Show resolved Hide resolved
scripts/base.js Outdated
const newCoordinates = newData.features[0].geometry.coordinates;
const newGeometryType = newData.features[0].geometry.type;

if (addCity) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Better is to invert the conditional and early return, like:

if (!addCity) {
  return
}

const newFile = ...

See https://medium.com/swlh/return-early-pattern-3d18a41bba8 about this tip.

Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

🙌

*
* @param map: The Leaflet map instance.
*/
const setUpParkingparkingLayer = async (map) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo in the function name?

@tunglinn tunglinn merged commit 13611ef into main Feb 4, 2024
1 check passed
@tunglinn tunglinn deleted the lazy-loading branch February 4, 2024 22:49
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.

2 participants