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: Frontend controlled map style #4937

Open
wants to merge 38 commits into
base: master
Choose a base branch
from

Conversation

marius-at-atb
Copy link
Contributor

@marius-at-atb marius-at-atb commented Jan 17, 2025

Closes https://github.com/AtB-AS/kundevendt/issues/19557
Closes https://github.com/AtB-AS/kundevendt/issues/10821
Closes https://github.com/AtB-AS/kundevendt/issues/19451
Closes https://github.com/AtB-AS/kundevendt/issues/19259

First of all excuse the large PR, but it is kinda needed to avoid incomplete/invalid steps along the way 😅

Feel free to ask me for closer explanations if needed!

What's happening in this PR:

  • The Map component has been updated. That means only the map tab and choose location maps are updated for now.
  • New hook: useMapboxJsonStyle, which uses the files inside mapbox-styles to get the style locally, but sprites from the url defined in remote config. This only shows geographical features, not NSR items.
  • New component: NationalStopRegistryFeatures. This is where the NSR items are shown now. In order to access the data, new mapbox env variables have been added.
  • New hooks for determining the style for icons and text for NSR and Shared Mobility items.
  • No longer zooming in on a path when selecting a feature.
  • Mobility components have been updated to use the new icons. These components will be removed when using the tile server, so let's not spend too much time on them..
  • Dark mode support.

How the zoom logic works:

  • In nsr-layers.ts, showAsDefaultAtZoomLevel is defined for each NSR layer. 3 zoom levels before, the minimized version will transition from small and invisible, to full size and visible. Then when showAsDefaultAtZoomLevel is reached, the icon switches to default. The label will then start to fade in as well.

Before:

After:

Acceptance Criteria:

  • Light theme in light mode
  • Dark theme in dark mode
  • 200% zoom
  • Performance - it should not be much slower than before
  • When selecting a map item, it should
    - [ ] show a custom pin to indicate the selected element
    - [ ] other items should become minimized (apart from scooter clusters, that will be addressed later)
  • When zooming in, the stop places should
    - [ ] first be invisible and tiny in the minimized state
    - [ ] then transition to reach full size and have no transparency in the minimized state as you zoom in
    - [ ] switch to the default icon
    - [ ] the label should then start becoming visible
    - [ ] The quays should start small, and when full size is reached, the text should start fading in
    - NOTE: different types of stop places are configured to reach these states at different zoom levels
  • Vehicles and stations should still work with clustering and show correct numbers (navigate to Oslo in the map to test this)

@marius-at-atb marius-at-atb self-assigned this Jan 17, 2025
setMapRegion((prevMapRegion) =>
isEqual(prevMapRegion, newMapRegion) ? prevMapRegion : newMapRegion,
);
if (!isEqual(mapRegion, newMapRegion)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Performance fix. setMapRegion always triggers a new component re-render, which re-trigger all the hooks. Since MapBox has a bug which triggers onMapIdle way too often, this causes a lot of side effects. If testing indicates that it is needed, we could consider throttling or debouncing as extra safety here.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe a stateless var instead. But perhaps is needed to be in that way.

Copy link
Member

Choose a reason for hiding this comment

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

Not really sure this has any effect 🤔 Since setting prevMapRegion should already not invoke rerender. It's not invoking the setter that causes rerender, but that the state has changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like this suggestion!
It is possible to change the mapRegion state to a ref instead, and update vehicles and stations directly instead of in a useEffect:

  const mapRegionRef = useRef<MapRegion>();
  const updateVehiclesAndStations = useCallback(
    (mapRegion: MapRegion) => {
      if (!mapRegion) return;
      mapRegionRef.current = mapRegion;
      updateRegionForVehicles?.(mapRegion);
      updateRegionForStations?.(mapRegion);
    },
    [updateRegionForVehicles, updateRegionForStations],
  );

and then instead of setMapRegion:

    if (!isEqual(mapRegionRef.current, newMapRegion)) {
      updateVehiclesAndStations(newMapRegion);
    }

Also for onDidFinishLoadingMap:

    updateVehiclesAndStations({
      visibleBounds,
      zoomLevel,
      center,
    });

Seems to work well when testing, so I actually prefer this.
However it is another change, so not sure if it should be included in this PR 🤔

@@ -184,6 +191,15 @@ export const Map = (props: MapProps) => {
async (feature: Feature) => {
if (!isFeaturePoint(feature)) return;

// should split components instead of this, ExploreLocation should only depend on location state, not features
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Upcoming PR..

{...MapViewConfig}
{...{
...MapViewConfig,
// only updating Map.tsx for now.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Upcoming PR for the other maps

@rosvik
Copy link
Member

rosvik commented Jan 23, 2025

Also: There is an override on the status bar color for the map tab that can be removed now. We should have light-content in dark mode and dark-content in light mode, just like elsewhere in the app.

@marius-at-atb
Copy link
Contributor Author

@rosvik Good catch with the status bar color! Updated in af01eeb. Let me know if I removed too much here.

@marius-at-atb
Copy link
Contributor Author

@tormoseng

  1. Will look into this a bit.
  2. Updated here so that the last listed item in nsrSymbolLayers is on top. Not 100% sure, but I think this is now the same order as in prod from before.
  3. We can look into this.
    4 and 5: Not a part of changes in this PR I think, but should be looked at 👍
    6: This has been addressed with onPress events directly on sources so that we don't need to wait for the querying of features where you clicked, and with useCallbacks/useMemos to minimize the number of renders of the Map component. However, it is still a bit slower than before because it supports more features. If still too slow, we probably go to the tile server directly.

@marius-at-atb
Copy link
Contributor Author

@gorandalum Two reasons for removing the "dotted line from your position to the selected stop" feature, after a discussion between me and @Aliaaen:

  • The auto-zooming took the user away from where they had navigated to in the map
  • There already is a button "Travel to" in the bottom sheet if this is what the user wants to know about

.. and of course technically it reduces complexity.

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.

5 participants