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

Add loading state and error message to Deployment map #705

Merged
merged 9 commits into from
Nov 19, 2024

Conversation

willemarcel
Copy link
Contributor

@willemarcel willemarcel commented Oct 31, 2024

Resolves #703

@LanesGood feel free to modify the style or text.

@willemarcel willemarcel requested a review from LanesGood October 31, 2024 10:37
Copy link
Contributor

@LanesGood LanesGood left a comment

Choose a reason for hiding this comment

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

@willemarcel I think this is looking good, but I'm seeing the error message on every campaign that doesn't have flight data. If I understand the code correctly, this should only appear if there is flight data, but it errors - is that correct? Should there be a geojson?.features?.length check combined with the geojsonError check?

EDIT
I attempted this, and then intentionally malformed the geojson for a campaign, but now I simply don't see the deployment map and there's no error message:
image

I also thought the loader should come first.

@willemarcel
Copy link
Contributor Author

Actually, the only error that can happen is that we don't have a geojson for the campaign. Should we modify the text to state more clearly that it wasn't an error, but the map data for that campaign is not available?

When running locally, you need to enable throttling in your network console to see the loading map component.

@LanesGood
Copy link
Contributor

@willemarcel looking again at this, I understand my confusion and think there needs to be some slight refactoring. The DeploymentMap is currently a child of the TimelineChart component. In TimelineChart the whole component is wrapped in a ReachUI Disclosure component to hide or show the timeline. This is behavior is unnecessary to wrap around the Deployment Map.

Instead, I would suggest the following:

  • Rename TimelineSection to DeploymentEventsSection
  • Rename Timeline directory within "Components" to DeploymentsEvents
  • Within this new DeploymentEventsSection, nest the existing TimelineChart and add a new child component, DeploymentMap
  • Move the existing MapErrorMsg and MapLoading components to the existing DeploymentMap to contain those concerns, and move the geojson fetching/validation logic to that as well

@LanesGood
Copy link
Contributor

New message and icon for empty geojson:
image

Should this be called MapErrorMsg or MapEmptyStateMsg? Bigger picture, is there a need to call the error type of the useFetch hook just for empty data, or could we simply set this message if the geoJson response returns null, and use a different/more accurate message if there is a true error?

@willemarcel
Copy link
Contributor Author

Should this be called MapErrorMsg or MapEmptyStateMsg?

I like MapEmptyStateMsg

Bigger picture, is there a need to call the error type of the useFetch hook just for empty data, or could we simply set this message if the geoJson response returns null, and use a different/more accurate message if there is a true error?

What we can do instead is check the status code of the request. If it's 404, the geojson file of that campaign was not found.

@LanesGood LanesGood self-requested a review November 19, 2024 16:25
@willemarcel willemarcel merged commit 471213a into develop Nov 19, 2024
5 checks passed
@willemarcel willemarcel deleted the feature/map-loading-state branch November 19, 2024 19:16
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.

Add loading state and error message to Deployment map
2 participants