Skip to content

RouteList: add bookmark icon to flagged routes #98

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.

Sign up for GitHub

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

Merged
incognitojam merged 8 commits into from
Mar 26, 2025
+15 −30
Merged

RouteList: add bookmark icon to flagged routes #98

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
36ad513
updated flag PR UI
ugtthis Mar 26, 2025
a35a023
Remove unused Avatar component
ugtthis Mar 26, 2025
c95650b
flag looks more like a badge
ugtthis Mar 26, 2025
18019d1
check timeline() before calling
ugtthis Mar 26, 2025
166045c
Merge branch 'master' into show-clearer-bookmarked-routes
ugtthis Mar 26, 2025
f4088b2
Merge remote-tracking branch 'origin/master' into show-clearer-bookma…
incognitojam Mar 26, 2025
7617406
revert
incognitojam Mar 26, 2025
1753afd
add suspense
incognitojam Mar 26, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 0 additions & 27 deletions src/components/material/Avatar.tsx
View file
Open in desktop

This file was deleted.

2 changes: 1 addition & 1 deletion src/components/material/Icon.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import clsx from 'clsx'
// biome-ignore format: the array should not be formatted
export const Icons = [
'add', 'arrow_back', 'camera', 'check', 'chevron_right', 'clear', 'close', 'description', 'directions_car', 'download', 'error',
'file_copy', 'info', 'menu', 'my_location', 'open_in_new', 'payments', 'person', 'progress_activity', 'satellite_alt', 'search',
'file_copy', 'flag', 'info', 'menu', 'my_location', 'open_in_new', 'payments', 'person', 'progress_activity', 'satellite_alt', 'search',
'settings', 'sync', 'upload', 'videocam',
] as const

Expand Down
12 changes: 11 additions & 1 deletion src/pages/dashboard/components/RouteList.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
import { createEffect, createResource, createSignal, For, Index, onCleanup, onMount, Suspense, type VoidComponent } from 'solid-js'
import { createEffect, createResource, createSignal, For, Index, onCleanup, onMount, Show, Suspense, type VoidComponent } from 'solid-js'
import dayjs from 'dayjs'

import { fetcher } from '~/api'
import { getTimelineStatistics } from '~/api/derived'
import Card, { CardContent, CardHeader } from '~/components/material/Card'
import Icon from '~/components/material/Icon'
import RouteStatistics from '~/components/RouteStatistics'
import { getPlaceName } from '~/map/geocode'
import type { RouteSegments } from '~/types'
Expand All @@ -19,6 +21,7 @@ const RouteCard: VoidComponent<RouteCardProps> = (props) => {
const endPosition = () => [props.route.end_lng || 0, props.route.end_lat || 0] as number[]
const [startPlace] = createResource(startPosition, getPlaceName)
const [endPlace] = createResource(endPosition, getPlaceName)
const [timeline] = createResource(() => props.route, getTimelineStatistics)
const [location] = createResource(
() => [startPlace(), endPlace()],
([startPlace, endPlace]) => {
Expand All @@ -41,6 +44,13 @@ const RouteCard: VoidComponent<RouteCardProps> = (props) => {
</div>
}
subhead={location()}
trailing={
<Show when={timeline()?.userFlags}>
Copy link
Copy Markdown
Collaborator

@incognitojam incognitojam Mar 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I think this should be wrapped in a <Suspense> otherwise it blocks the RouteCard from showing until the timeline is fetched

Copy link
Copy Markdown
Contributor Author

@ugtthis ugtthis Mar 26, 2025

Choose a reason for hiding this comment

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

I dont think suspense is needed here because:

  • We're simply conditionally showing the flag icon only after the data loads - using timeline()?.userFlags with <Show> automatically handles the pending state by showing nothing until the data arrives
  • RouteCard won't be blocked because in SolidJS, resources are granular - the rest of the component continues rendering while timeline() loads. Only the specific UI element that depends on timeline()?.userFlags waits
  • Suspense would be needed if we wanted to show a loading indicator instead of nothing while the data loads

Let me know what you think

Copy link
Copy Markdown
Collaborator

@incognitojam incognitojam Mar 26, 2025

Choose a reason for hiding this comment

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

When I scroll the route list right now on the live site, I can see the stats loading, but on this PR when I scroll it shows the whole card loading instead

Copy link
Copy Markdown
Contributor Author

@ugtthis ugtthis Mar 26, 2025

Choose a reason for hiding this comment

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

• I think the root issue was "Engaged" statistic was accessing timeline data directly without checking if it exists
• Fixed by adding timeline() ? check before calling formatEngagement
• This prevents rendering from blocking while timeline data loads
• Cards now render immediately with statistics loading independently, just like on the live site

The PR site seems to be loading as expected when I checked. let me know if you see differently

Copy link
Copy Markdown
Collaborator

@incognitojam incognitojam Mar 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

It's still doing the same thing

Fixed by adding timeline() ? check before calling formatEngagement

Calling a resource like timeline() is what triggers suspense, and the next parent <Suspense> component is in <RouteList>

<div class="flex items-center justify-center rounded-full bg-gradient-to-br from-amber-500 to-amber-900 p-2 ring-1 ring-amber-300 shadow-inner shadow-black/20">
<Icon class="text-yellow-300" size="20" name="flag" filled />
</div>
</Show>
}
/>

<CardContent>
Expand Down
Loading