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

feat: Zoom to select area #52

Merged
merged 14 commits into from
Sep 24, 2024
Merged

feat: Zoom to select area #52

merged 14 commits into from
Sep 24, 2024

Conversation

oliverroick
Copy link
Collaborator

Implements #24

Upon selecting an area, we're not just passing the areaId but the whole Feature; in the area-select action we're extracting the area ID and bounding box and put both in store.

Other changes

  • Adds @turf/bbox to calculated the bounding box from the geometry
  • Add effect hook to globe to set the bounding box on the map

Refactoring

I cleaned up the xstate related code:

  • Changed the naming of actions and events so events and actions are a bit more differentiable
  • Moved the context, events, and actions types to its on module to make the machine module a bit more readable
  • Removed anything related to loading the country geojson

@oliverroick oliverroick requested a review from vgeorge September 19, 2024 01:04
Copy link
Collaborator

@vgeorge vgeorge left a comment

Choose a reason for hiding this comment

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

@oliverroick some comments on this:

Fit area to bounds

I kept the area GeoJSON code because we cannot rely on the geometry returned by queryRenderedFeature to get the area bounding box (bbox). If the vector tile doesn't contain the full geometry, it will zoom to the tile bounds:

zoom-to-area

I suggest keeping the current implementation or open a ticket to track this until we have an API route to query the area bounds.

Highlighted area by filtering the id

I made a change to use the area ID instead of the queried feature to draw the area outline. Due to the same issue mentioned above, the highlighted area would be the tile bbox if the area is not fully visible. By using the area ID, the full feature is outlined.

Highlighted area render performance

I thought we had solved this issue in #37, but it still persists. I don't think this is a blocker for this PR, so we can open a ticket for it.

XState conventions

I often rely on the machine diagram to make changes, it is much easier that editing the machine definition. When the machine gets larger, it becomes difficult to modify it by just looking at the code, but relatively easy to do through the XState editor, which also has a VSCode extension.

My preference is to keep the more verbose convention as it looks more readable in the diagram than the concise version. Here's a comparison:

Screenshot 2024-09-19 at 12 14 14 Screenshot 2024-09-19 at 12 15 50

XState has good typescript support and, in most cases, you won't have to worry with typing the full action name because of typescription autocomplete. Here is an example with Copilot disabled:

naming

XState VSCode Extension

To my surprise, this extension is not working. There are a couple of open issues about this on their GitHub repo (statelyai/xstate-tools#325 and statelyai/xstate-tools#468). In our case, it’s not working because the machine is under a folder with parentheses in its path (src/app/(home)/globe/state/index.tsx).

Editing the machine without the diagram editor is quite unpleasant. I'm wondering if we could move it to a dedicated folder like src/app/machines/globe. We could also consider moving the components of src/app/(home) back to the root level (src/app/index.tsx), since we now know the app won't have subpages that don't include the globe.

@oliverroick
Copy link
Collaborator Author

Fit area to bounds

I kept the area GeoJSON code because we cannot rely on the geometry returned by queryRenderedFeature to get the area bounding box (bbox). If the vector tile doesn't contain the full geometry, it will zoom to the tile bounds:

I suggest keeping the current implementation or open a ticket to track this until we have an API route to query the area bounds.

But we’ve never used the GeoJSON in the actual app, which is why I’ve removed it. It feels wrong to keep all this data in memory just so we can query the geometry occasionally.

I tried to solve this but ran into my limits of my understanding how the state machine with xstate should be configured.

Here’s how I approached it:

  • I extended the query in area/[areaId]/page so it returns the geometry bbox for the area.
  • From the page I then want to trigger an xstate event that contains the bbox, and the subsequent action would update the state.

There will be two states related to this scenario:

  1. Country is selected
  2. Map bounding box is set

Now “Country is selected” is a state that we can have with or without the map bounding box. Likewise the map bounding box can be set when now country is selected; for example, when a route or node is selected. I feels like we can have several states in parallel, which is contrary to how I understand state machines. In a state machine, there should one state at a time. So how can we model this in the state machine?

Highlighted area by filtering the id

I made a change to use the area ID instead of the queried feature to draw the area outline. Due to the same issue mentioned above, the highlighted area would be the tile bbox if the area is not fully visible. By using the area ID, the full feature is outlined.

Highlighted area render performance

I thought we had solved this issue in #37, but it still persists. I don't think this is a blocker for this PR, so we can open a ticket for it.

The issue regressed because of the update you’ve made. Filtering across the whole area dataset is too expensive and should be avoided, which is why I have removed it based on the initial review. I agree, this is a bug that we need to fix, but it’s a separate issue unrelated to the work we’re discussing here. I’ve opened a ticket.

XState conventions

I often rely on the machine diagram to make changes, it is much easier that editing the machine definition. When the machine gets larger, it becomes difficult to modify it by just looking at the code, but relatively easy to do through the XState editor, which also has a VSCode extension.

My preference is to keep the more verbose convention as it looks more readable in the diagram than the concise version.

From the labels you suggested, I can never tell whether I’m looking at an action or an event or a state. I couldn’t get the visual editor to work, which is why I made the change. The naming I proposed, is a lot clearer as it follows a simple convention: type:target:change so you can quickly process what is happening and what part of the state is affected. If we introduce more actions/events it will be harder to make sense of the naming written in verbose prose. I don’t agree that he naming in prose is easier to process in the visual editor once you understand my convention. To me, there’s no difference between Area is selected and area:selected for the states.

Also, I’d argue there’s a long-standing convention in software development to use unambiguous names for identifiers instead of conversational English.

XState VSCode Extension

To my surprise, this extension is not working. There are a couple of open issues about this on their GitHub repo (statelyai/xstate-tools#325 and statelyai/xstate-tools#468). In our case, it’s not working because the machine is under a folder with parentheses in its path (src/app/(home)/globe/state/index.tsx).

Editing the machine without the diagram editor is quite unpleasant. I'm wondering if we could move it to a dedicated folder like src/app/machines/globe. We could also consider moving the components of src/app/(home) back to the root level (src/app/index.tsx), since we now know the app won't have subpages that don't include the globe.

We can make this change, but it’s not related to the ticket we’re discussing here. I think, we should tackle that in a separate PR.

@vgeorge vgeorge self-requested a review September 23, 2024 07:23
@vgeorge
Copy link
Collaborator

vgeorge commented Sep 23, 2024

@oliverroick, your arguments make sense. We should use a more structured convention for naming the elements in the machine.

I found a solution for the hovering issue; it should now be snappy and highlight the full geometry. It involved using map.setFeatureState, as shown in this example from the Mapbox docs, and adding an integer ID to the MVT features to make the map query more performant:

hover

I also added the following changes:

  • Renamed the globe component to map and moved it to the components folder, fixing the diagram viewer issue.
  • Moved event and action types to separate files for easier access and maintainability
  • Changed the mousemove event handler to trigger events in XState. This is a pattern I used in Pearl to keep the map component as 'dumb' as possible, making it easier to handle map effects at different states of the app

Next steps should be:

  • Add a new route that returns the area and its bounding box. This is required to avoid using the vector tile feature bbox, which is not always fully loaded
  • Apply the correct bounding box on click
  • Apply the correct bounding box when accessing the route /area/:id

@wrynearson, as discussed, I'll add changes to this while Oliver is out and will ask for your review when it’s ready.

@vgeorge
Copy link
Collaborator

vgeorge commented Sep 24, 2024

@wrynearson this is ready for review, here is a screen capture:

zoom on click

How to test:

  • Start the server and visit http://localhost:3000
  • Move the mouse point over the map, the hovered area should have a different style and a tooltip is shown
  • Click on a area, the map should zoom to it
  • Click on another area, the map should zoom to it

Know issues: the side panel is not displayed yet and it is not possible yet to visit an area URL to get an area view. This is a change we should address on a separated PR.

@vgeorge vgeorge requested review from wrynearson and removed request for vgeorge September 24, 2024 12:20
Copy link
Collaborator

@wrynearson wrynearson left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @vgeorge! The functionality works as described.

One note for the future - we may have to adjust the zoom to be closer to be closer to what's in the designs. For example, this PR zooms to a higher zoom level than what's in the designs. cc @faustoperez

image image

@vgeorge
Copy link
Collaborator

vgeorge commented Sep 24, 2024

@wrynearson thanks for the review! I increased the padding to give more breathing room to the selected area. Let's revisit this later when there are more elements on the page, as we'll probably need to adjust further.

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.

3 participants