Skip to content
This repository has been archived by the owner on Dec 7, 2022. It is now read-only.

Bugfix - Quick map fix for null entries #108

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

Conversation

babbitt
Copy link

@babbitt babbitt commented Mar 29, 2021

This is to address #103. Not the best code in the world but concise. Removes null entries to prevent map from putting locations on "null island"

Before:
image
After:
image

I know this isn't the best JS in the world, so do let me know if mapbox or CARTO provides a better way of doing this (I couldn't find any)

Happy and excited to help!

@babbitt
Copy link
Author

babbitt commented Mar 29, 2021

Apologies for all these linting commits. ESlint was not playing ball with me today.

Comment on lines +88 to +90
...stateData.features.filter(
(feature) => feature.geometry.coordinates[0] != null
)

Choose a reason for hiding this comment

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

Is there a way we can ensure the stateData on the store's end is clean from the store's level? It may not be helpful to have null locations in general (think of another component that uses this.$store.state.usStates.usState;)

Copy link
Author

Choose a reason for hiding this comment

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

Totally agree with you. I don't have a great idea of the project layout as I just cloned and started working on it an hour ago but I figured that list is useful with nulls because it also informs the list on the side (from what I can tell). So based on that I would imagine we still want those items to show up in the list view, just not on the map if they don't have coordinates.

Copy link
Author

Choose a reason for hiding this comment

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

Actually I just realized this would break the list since it's modifying the data directly. Whoops. @Maker-Mark let me know (if you can) if my hunch was right and if so I'll change this so it uses a copied version for the map.

Copy link
Author

Choose a reason for hiding this comment

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

I have a deep copy of this code ready to go. So if we replace the stateData assignment with

JSON.parse(
        JSON.stringify(this.$store.state.usStates.usState)
      );

we'll have a deep copy which keeps the unmappable items in the list but not on the map.

Choose a reason for hiding this comment

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

I'm trying to figure out where the data is being fetched from. If we can sanitize it there, we should be able to get rid of them

Choose a reason for hiding this comment

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

If we just want to fix it for locations. We can just filter the list instead of splicing the data. Filter will return a new array.

Copy link
Author

Choose a reason for hiding this comment

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

I'm trying to figure out where the data is being fetched from. If we can sanitize it there, we should be able to get rid of them

Sorry just saw this. It comes from the API directly to my knowledge

await this.$http.$get(`/api/v0/states/${this.$route.params.state}.json`)

Copy link
Author

Choose a reason for hiding this comment

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

If we just want to fix it for locations. We can just filter the list instead of splicing the data. Filter will return a new array.

Agreed. I think the issue there is that we also need to pass all the metadata along with it. We can just point to the same metadata since there's no issue with store_count being incorrect but since it's a negligible and we'd have to make that second object anyways I figured the deep copy is worth it.

Choose a reason for hiding this comment

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

Just noticed this discussion after I started working on #141. I believe mapbox does not rely on metadata and I could possibly add a null check to the filteredLocationData, would that help?

@babbitt
Copy link
Author

babbitt commented Apr 6, 2021

@GUI According to #103 I'm not exactly sure if this has already been fixed. Can you clarify? If so I'll close this out. Otherwise happy to bring it up to date with conflicts

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

Successfully merging this pull request may close these issues.

3 participants