Feat/stop-route-labels-256

[Ahmedhossamdev]

Fix: #256

[Copilot]

Pull Request Overview

This PR adds route labels functionality to stop markers on the map, addressing issue #256. The feature displays route names on stop markers when zoomed in sufficiently (zoom level 16+).

• Adds zoom-based visibility toggle for route labels on stop markers
• Implements expandable/collapsible route labels with a maximum of 3 visible routes initially
• Adds comprehensive test coverage for the new route labels functionality

Reviewed Changes

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

File	Description
src/lib/Provider/OpenStreetMapProvider.svelte.js	Adds zoom threshold tracking and batch marker updates for route label visibility
src/lib/Provider/GoogleMapProvider.svelte.js	Implements similar zoom-based route label functionality for Google Maps
src/components/map/StopMarker.svelte	Adds route label UI with expandable functionality and positioning logic
src/components/tests/RouteLabels.test.js	Comprehensive test suite covering route label behavior and zoom logic

Comments suppressed due to low confidence (1)

src/lib/Provider/GoogleMapProvider.svelte.js:1

• The marker object structure has been changed to include props, but the props object is created earlier in the function and stored in a separate variable. Consider ensuring the props reference in markerObj stays synchronized if props is modified elsewhere, or document that props should not be mutated after this point.

import { loadGoogleMapsLibrary, createMap, nightModeStyles } from '$lib/googleMaps';

---

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

[coveralls]

coverage: 76.647% (+0.4%) from 76.292%
when pulling 0483514 on feat/stop-route-labels-256
into f7f170d on main.

[aaronbrethorst]

Hey Ahmed! Nice work! I have a few items to address before we merge:

Critical (Please Fix)

1. Accessibility: Clickable div needs keyboard support

src/components/map/StopMarker.svelte

The route label is clickable but not keyboard accessible. Screen reader and keyboard users can't expand the route list.

<!-- Current -->
<!-- svelte-ignore a11y_click_events_have_key_events -->
<!-- svelte-ignore a11y_no_static_element_interactions -->
<div
    class="routes-label ..."
    onclick={toggleRoutesList}
>

Suggested fix - convert to a button:

<button
    type="button"
    class="routes-label {isExpanded ? 'expanded' : ''} position-{labelPosition}"
    onclick={toggleRoutesList}
    aria-expanded={isExpanded}
    aria-label={isExpanded ? 'Collapse route list' : `Show all ${routeNames.length} routes`}
>

You'll need to add all: unset; or similar to the .routes-label styles to reset button defaults.

2. Expanded state persists across zoom levels

If a user expands the route list at zoom 16+, zooms out below 16, then zooms back in, the label is still expanded. This creates confusing UX.

Suggested fix - add an effect to reset state:

$effect(() => {
    if (!showRoutesLabel) {
        isExpanded = false;
    }
});

Important (Should Fix)

3. Dark mode text color inconsistency

src/components/map/StopMarker.svelte

The expanded state uses hardcoded colors and :global(.dark) which conflicts with the Tailwind dark mode classes used elsewhere:

.routes-label.expanded {
    color: #1f2937;  /* Overrides dark:text-white from line 159 */
}

:global(.dark) .routes-label.expanded {
    color: #ffffff;
}

Suggested fix - use Tailwind consistently:

.routes-label.expanded {
    @apply font-semibold;
    @apply bg-brand/10 dark:bg-neutral-700;
    @apply border-brand-secondary dark:border-brand;
    @apply text-gray-800 dark:text-white;
    white-space: normal;
    max-width: 250px;
}

Then remove the :global(.dark) block.

4. Missing test for collapse functionality

src/components/tests/RouteLabels.test.js

The tests verify expanding the label but not collapsing it back. Consider adding something like:

test('collapses back to 3 routes when clicked again', async () => {
    const user = userEvent.setup();
    const onClick = vi.fn();

    render(StopMarker, {
        props: {
            stop: mockStopWithManyRoutes,
            onClick,
            icon: faBus,
            showRoutesLabel: true
        }
    });

    const routeLabel = screen.getByText(/7, 10, 49/).closest('.routes-label');

    // Expand
    await user.click(routeLabel);
    expect(screen.getByText(/345/)).toBeInTheDocument();

    // Collapse
    await user.click(routeLabel);
    expect(screen.queryByText('345')).not.toBeInTheDocument();
    expect(screen.getByText('⋯')).toBeInTheDocument();
});

[Ahmedhossamdev]

@aaronbrethorst Working on it...

[aaronbrethorst]

great work, thanks for getting those changes in!