Skip to content

Conversation

@Alessandro100
Copy link
Contributor

@Alessandro100 Alessandro100 commented Oct 27, 2025

Summary:

The GTFS Visualization file was large (1000+ lines), making quick changes going forward and understanding it after time of not seeing it will be difficult. This PR aims to reduce the complexity of the component. At the same time, during this refactor a bug was fixed and UX enhancements applied. Eslint is also re-enabled on the GTFS Visualization files

Expected behavior:

The map component should behave the way it was before with the following enhancements

UX enhancements

  • Selecting routes is more forgiving, the user will not have to be so precise
  • Selecting stops is also more forgiving, the user will not have to be so precise

Bug Fixes

  • Clicking on a stop would not show the popup for stop data. Clicking on a stop is now working again
  • Clicking on a route would select other routes (ex: clicking on route "188" would activate route "1"). Clicking on a route will now only select that route

Testing tips:

Go to any map and test the general functionality. Especially clicking on stops, hovering routes and clicking on routes assuring no extra pop up

Next steps

  • Writing tests
  • Extracting styles

Please make sure these boxes are checked before submitting your pull request - thanks!

  • Run the unit tests with ./scripts/api-tests.sh to make sure you didn't break anything
  • Add or update any needed documentation to the repo
  • Format the title like "feat: [new feature short description]". Title must follow the Conventional Commit Specification(https://www.conventionalcommits.org/en/v1.0.0/).
  • Linked all relevant issues
  • Include screenshot(s) showing how this pull request works and fixes the issue(s)

@Alessandro100 Alessandro100 self-assigned this Oct 27, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the GTFS Visualization Map component to improve maintainability and address UX/functionality issues. The 1000+ line component has been split into smaller, focused modules while fixing critical bugs related to stop selection and route filtering.

Key Changes:

  • Component extracted into separate files for layers, functions, and UI panels
  • Fixed stop click interaction bug that prevented popup display
  • Improved route selection precision to prevent unintended multi-route activation
  • Enhanced click target areas for stops and routes

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
GtfsVisualizationMap.tsx Core component refactored with extracted logic and improved null checks
GtfsVisualizationMap.layers.tsx Map layer definitions extracted from main component
GtfsVisualizationMap.functions.tsx Utility functions for route ID extraction, bounds calculation, and color generation
SelectedRoutesStopsPanel.tsx Draggable panel component for displaying selected route stops
ScanningOverlay.tsx Overlay component for tile scanning progress indication

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

if (!el.isStop) {
const routeElement: MapRouteElement = el as MapRouteElement;
if (routeElement.routeId && routeElement.routeColor) {
if (routeElement.routeId !== '' && routeElement.routeColor !== '') {
Copy link

Copilot AI Oct 27, 2025

Choose a reason for hiding this comment

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

The null/undefined check is incomplete. Consider using routeElement.routeId && routeElement.routeColor or routeElement.routeId != null && routeElement.routeColor != null to handle both null/undefined and empty string cases consistently.

Suggested change
if (routeElement.routeId !== '' && routeElement.routeColor !== '') {
if (
routeElement.routeId != null &&
routeElement.routeId !== '' &&
routeElement.routeColor != null &&
routeElement.routeColor !== ''
) {

Copilot uses AI. Check for mistakes.
'source-layer': 'routesoutput',
type: 'line',
paint: {
'line-color': theme.palette.background.default,
Copy link

Copilot AI Oct 27, 2025

Choose a reason for hiding this comment

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

React hooks like useTheme() cannot be called inside regular functions. The theme should be passed as a parameter to RoutesWhiteLayer instead of being called within the function body.

Copilot uses AI. Check for mistakes.
Comment on lines 189 to 190
): LayerSpecification => {
const theme = useTheme();
Copy link

Copilot AI Oct 27, 2025

Choose a reason for hiding this comment

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

React hooks like useTheme() cannot be called inside regular functions. The theme should be passed as a parameter to StopsHighlightOuterLayer instead of being called within the function body.

Suggested change
): LayerSpecification => {
const theme = useTheme();
theme: any,
): LayerSpecification => {

Copilot uses AI. Check for mistakes.
Comment on lines 237 to 238
'circle-opacity': 0,
'circle-radius': 5,
Copy link

Copilot AI Oct 27, 2025

Choose a reason for hiding this comment

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

The StopsIndexLayer uses circle-opacity: 0 to create invisible click targets, but the description mentions making stops more forgiving to select. The radius of 5 is now larger than the previous value of 1, which improves UX as intended. Consider adding a comment explaining this is an invisible layer for enhanced click detection.

Copilot uses AI. Check for mistakes.
type: 'line',
paint: {
'line-color': theme.palette.background.default,
'line-width': ['match', ['get', 'route_type'], '3', 10, '1', 15, 3],
Copy link

Copilot AI Oct 27, 2025

Choose a reason for hiding this comment

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

The line width values (10, 15, 3) differ from the original code which used (4, 15, 3). This change to route type '3' width from 4 to 10 is a significant visual change that isn't mentioned in the PR description. Consider documenting this intentional change or verifying it's correct.

Suggested change
'line-width': ['match', ['get', 'route_type'], '3', 10, '1', 15, 3],
'line-width': ['match', ['get', 'route_type'], '3', 4, '1', 15, 3],

Copilot uses AI. Check for mistakes.
@github-actions
Copy link

github-actions bot commented Oct 27, 2025

*Lighthouse ran on https://mobility-feeds-dev--pr-1419-k4dwm61b.web.app/ * (Desktop)
⚡️ HTML Report Lighthouse report for the changes in this PR:

Performance Accessibility Best Practices SEO
🟠 85 🟢 100 🟢 100 🟢 100

*Lighthouse ran on https://mobility-feeds-dev--pr-1419-k4dwm61b.web.app/feeds * (Desktop)
⚡️ HTML Report Lighthouse report for the changes in this PR:

Performance Accessibility Best Practices SEO
🟠 57 🟢 91 🟢 100 🟢 100

*Lighthouse ran on https://mobility-feeds-dev--pr-1419-k4dwm61b.web.app/feeds/gtfs/mdb-2126 * (Desktop)
⚡️ HTML Report Lighthouse report for the changes in this PR:

Performance Accessibility Best Practices SEO
🟠 57 🟢 95 🟢 100 🟢 100

*Lighthouse ran on https://mobility-feeds-dev--pr-1419-k4dwm61b.web.app/feeds/gtfs_rt/mdb-2585 * (Desktop)
⚡️ HTML Report Lighthouse report for the changes in this PR:

Performance Accessibility Best Practices SEO
🟠 88 🟠 88 🟢 100 🟢 100

*Lighthouse ran on https://mobility-feeds-dev--pr-1419-k4dwm61b.web.app/gbfs/gbfs-flamingo_porirua * (Desktop)
⚡️ HTML Report Lighthouse report for the changes in this PR:

Performance Accessibility Best Practices SEO
🟢 100 🟢 100 🟢 100 🟢 100

@github-actions
Copy link

Preview Firebase Hosting URL: https://mobility-feeds-dev--pr-1419-k4dwm61b.web.app

@Alessandro100 Alessandro100 force-pushed the feat/gtfs-visualization-refactor-touchups branch from ffa4ee6 to af6c1b6 Compare October 29, 2025 15:05
Copy link
Contributor

@cka-y cka-y left a comment

Choose a reason for hiding this comment

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

LGTM 💯

@Alessandro100 Alessandro100 merged commit 0b4e1ef into main Oct 29, 2025
4 checks passed
@Alessandro100 Alessandro100 deleted the feat/gtfs-visualization-refactor-touchups branch October 29, 2025 17:12
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