From e84e816b56a1ad942c8e30c76f3df952459e631d Mon Sep 17 00:00:00 2001 From: Jack Greenlee Date: Tue, 20 Feb 2024 13:02:04 -0500 Subject: [PATCH 01/14] fix crash on updatePrefReminderTime `newTime` here is not an ISO string; it is a JavaScript Date object. Attempting to parse it as an ISO string results in "Invalid DateTime" being stored to the user prefs and a crash on the Profile screen --- www/js/control/ProfileSettings.tsx | 4 ++-- www/js/splash/notifScheduler.ts | 6 +++++- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/www/js/control/ProfileSettings.tsx b/www/js/control/ProfileSettings.tsx index 0c6101c48..4a33a6a68 100644 --- a/www/js/control/ProfileSettings.tsx +++ b/www/js/control/ProfileSettings.tsx @@ -297,10 +297,10 @@ const ProfileSettings = () => { if (!uiConfig?.reminderSchemes) return logWarn('In updatePrefReminderTime, no reminderSchemes yet, skipping'); if (storeNewVal) { - const m = DateTime.fromISO(newTime); + const dt = DateTime.fromJSDate(newTime); // store in HH:mm setReminderPrefs( - { reminder_time_of_day: m.toFormat('HH:mm') }, + { reminder_time_of_day: dt.toFormat('HH:mm') }, uiConfig.reminderSchemes, isScheduling, setIsScheduling, diff --git a/www/js/splash/notifScheduler.ts b/www/js/splash/notifScheduler.ts index b059aa3cd..00f24571d 100644 --- a/www/js/splash/notifScheduler.ts +++ b/www/js/splash/notifScheduler.ts @@ -24,7 +24,11 @@ const calcNotifTimes = (scheme, dayZeroDate, timeOfDay): DateTime[] => { .plus({ days: d }) .toFormat('yyyy-MM-dd'); const notifTime = DateTime.fromFormat(date + ' ' + timeOfDay, 'yyyy-MM-dd HH:mm'); - notifTimes.push(notifTime); + if (notifTime.isValid) { + notifTimes.push(notifTime); + } else { + displayErrorMsg('Cannot schedule notifs with invalid time of day: ' + timeOfDay); + } } } return notifTimes; From 9be27b13b468d03369da9eecdc85f0907a05b7c1 Mon Sep 17 00:00:00 2001 From: Jack Greenlee Date: Wed, 21 Feb 2024 13:43:22 -0500 Subject: [PATCH 02/14] update "short" demo survey so people born in 2006 can take it The short version of the demographics survey asks for year of birth and we restrict the valid range of responses so as to not allow participants younger than 18. Last year when I uploaded this, I wrote in 2005 as the upper limit. But theoretically, people born in 2006 can now participate in those studies. I found a more generic solution using the ODK forms constraint syntax and some built-in functions they have: `. > 1900 and . + 18 <= format-date(today(), '%Y')` So now we won't have to update this every year; it actually takes into account the current year when validating the form. --- www/json/demo-survey-short-v1.json | 2 +- www/json/demo-survey-short-v1.xml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/www/json/demo-survey-short-v1.json b/www/json/demo-survey-short-v1.json index 20b02817f..6ec5e271f 100644 --- a/www/json/demo-survey-short-v1.json +++ b/www/json/demo-survey-short-v1.json @@ -1 +1 @@ -{"languageMap":{"Spanish (es)":"es","English (en)":"en"},"form":"
\n

OpenPATH Short Demographics Survey

\n \n \n

Información de nivel personalPersonal Level Information

¿Cuál describe de la mejor manera su género?What best describes your gender?*\n
This field is required
¿Cuál es su raza/etnicidad?What is your race/ethnicity?*Por favor seleccione todas las respuestas válidas.Please select all that apply.\n
This field is required
¿Tiene una licencia de conducir válida?Do you have a valid drivers license?*\n
This field is required
¿Cuál es el grado más alto o el título que ha obtenido?What is the highest level of education you have completed?*\n
This field is required
\n
\n

Información a nivel del hogarHousehold Level Information

¿Cuál es tu tipo de casa?What is your home type?*\n
This field is required
Por favor, identifique qué categoría representa el ingreso total de su hogar, antes de impuestos, en el último año.Please identify which category represents your total household income, before taxes, for last year.*Preguntamos esto porque el ingreso está relacionado con cómo, cuándo y por qué la gente va de un lugar a otro. Esta información se utilizará únicamente con fines estadísticos.We are asking this because income is related to how, when and why people go from place to place. This information will be used for statistical purposes only.\n
This field is required
Incluyéndose usted, ¿cuántas personas viven en su casa?Including yourself, how many people live in your home?*\n
This field is required
¿Cuántos vehículos de motor son propiedad, están alquilados o están disponibles para uso regular por las personas que vive actualmente en su hogar?How many motor vehicles are owned, leased, or available for regular use by the people who currently live in your household?*Incluya motocicletas, ciclomotores y vehículos recreativos.Include motorcycles, mopeds and RVs.\n
This field is required
\n
\n \n
","model":"\n \n \n \n \n \n \n \n \n \n \n \n \n \n \n \n \n \n \n \n \n ","transformerVersion":"2.1.5"} \ No newline at end of file +{"form":"
\n

OpenPATH Short Demographics Survey

\n\n \n \n

\nInformación de nivel personalPersonal Level Information\n

\n
\n
\n\n¿Cuál describe de la mejor manera su género?What best describes your gender?*\n \n
\n\n
\n
\nThis field is required\n
\n
\n
\n\n¿Cuál es su raza/etnicidad?What is your race/ethnicity?*Por favor seleccione todas las respuestas válidas.Please select all that apply.\n \n
\n\n
\n
\nThis field is required\n
\n
\n
\n\n¿Tiene una licencia de conducir válida?Do you have a valid drivers license?*\n \n
\n\n
\n
\nThis field is required\n
\n
\n
\n\n¿Cuál es el grado más alto o el título que ha obtenido?What is the highest level of education you have completed?*\n \n
\n\n
\n
\nThis field is required\n
\n
\n

\nInformación a nivel del hogarHousehold Level Information\n

\n
\n
\n\n¿Cuál es tu tipo de casa?What is your home type?*\n \n
\n\n
\n
\nThis field is required\n
\n
\n
\n\nPor favor, identifique qué categoría representa el ingreso total de su hogar, antes de impuestos, en el último año.Please identify which category represents your total household income, before taxes, for last year.*Preguntamos esto porque el ingreso está relacionado con cómo, cuándo y por qué la gente va de un lugar a otro. Esta información se utilizará únicamente con fines estadísticos.We are asking this because income is related to how, when and why people go from place to place. This information will be used for statistical purposes only.\n \n
\n\n
\n
\nThis field is required\n
\n
\n
\n\nIncluyéndose usted, ¿cuántas personas viven en su casa?Including yourself, how many people live in your home?*\n \n
\n\n
\n
\nThis field is required\n
\n
\n
\n\n¿Cuántos vehículos de motor son propiedad, están alquilados o están disponibles para uso regular por las personas que vive actualmente en su hogar?How many motor vehicles are owned, leased, or available for regular use by the people who currently live in your household?*Incluya motocicletas, ciclomotores y vehículos recreativos.Include motorcycles, mopeds and RVs.\n \n
\n\n
\n
\nThis field is required\n
\n
\n \n
\n\n
\n
","model":"\n \n \n \n \n \n \n \n \n \n \n \n \n \n \n \n \n \n \n \n \n \n \n \n \n","languageMap":{"Spanish (es)":"es","English (en)":"en"},"transformerVersion":"4.0.0"} diff --git a/www/json/demo-survey-short-v1.xml b/www/json/demo-survey-short-v1.xml index bc723f5ee..1f4ce1b36 100644 --- a/www/json/demo-survey-short-v1.xml +++ b/www/json/demo-survey-short-v1.xml @@ -430,7 +430,7 @@ - + From 6f101d120f50773b499501464cba9129cbbf856e Mon Sep 17 00:00:00 2001 From: Jijeong Lee Date: Thu, 22 Feb 2024 02:16:40 -0800 Subject: [PATCH 03/14] Add FlashList props to prevent blank space when scrolling --- www/js/diary/list/TimelineScrollList.tsx | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/www/js/diary/list/TimelineScrollList.tsx b/www/js/diary/list/TimelineScrollList.tsx index 06fd149c0..f705f56f8 100644 --- a/www/js/diary/list/TimelineScrollList.tsx +++ b/www/js/diary/list/TimelineScrollList.tsx @@ -97,6 +97,25 @@ const TimelineScrollList = ({ } ListFooterComponent={isLoading == 'prepend' ? smallSpinner : footer} ItemSeparatorComponent={separator} + /* Percent of viewport that must be covered for a partially occluded item to count as "viewable", 0-100. + a single pixel in the viewport makes the item viewable */ + viewabilityConfig={{ + viewAreaCoveragePercentThreshold: 0, + itemVisiblePercentThreshold: 0, + waitForInteraction: false, + }} + // 'getItemType' prop is used when FlashList has different types of cell components and these are vastly different + getItemType={(item) => { + if (item.origin_key.includes('trip')) { + return 'trip'; + } else if (item.origin_key.includes('place')) { + return 'place'; + } else if (item.origin_key.includes('untracked')) { + return 'untracked'; + } else { + return 'unknown'; + } + }} /> ); } else { From f9115ab6bbf198d47abd2abd30229ed9cfd7f59b Mon Sep 17 00:00:00 2001 From: Jijeong Lee Date: Mon, 26 Feb 2024 13:40:35 -0800 Subject: [PATCH 04/14] cache leaflet map to reduce the cost for creating a map --- www/js/components/LeafletView.tsx | 50 +++++++++++++++++++++++-------- 1 file changed, 37 insertions(+), 13 deletions(-) diff --git a/www/js/components/LeafletView.tsx b/www/js/components/LeafletView.tsx index 822719654..f9854740c 100644 --- a/www/js/components/LeafletView.tsx +++ b/www/js/components/LeafletView.tsx @@ -1,21 +1,27 @@ import React, { useEffect, useRef, useState } from 'react'; import { View } from 'react-native'; import { useTheme } from 'react-native-paper'; -import L, { Map } from 'leaflet'; +import L, { Map as LeafletMap } from 'leaflet'; import { GeoJSONStyledFeature } from '../types/diaryTypes'; +import parse from 'html-react-parser'; const mapSet = new Set(); +const cachedLeafletMap = new Map(); + export function invalidateMaps() { mapSet.forEach((map) => map.invalidateSize()); } const LeafletView = ({ geojson, opts, ...otherProps }) => { const mapElRef = useRef(null); - const leafletMapRef = useRef(null); + const leafletMapRef = useRef(null); const geoJsonIdRef = useRef(null); const { colors } = useTheme(); + // non-alphanumeric characters are not safe for element IDs + const mapElId = `map-${geojson.data.id.replace(/[^a-zA-Z0-9]/g, '')}`; + const [isMapReady, setIsMapReady] = useState(false); - function initMap(map: Map) { + function initMap(map: LeafletMap) { L.tileLayer('https://tile.openstreetmap.org/{z}/{x}/{y}.png', { attribution: '© OpenStreetMap', opacity: 1, @@ -33,27 +39,41 @@ const LeafletView = ({ geojson, opts, ...otherProps }) => { } useEffect(() => { + // if a Leaflet map is chached, there is no need to create the map again + if (cachedLeafletMap.has(mapElId)) return; // if a Leaflet map already exists (because we are re-rendering), remove it before creating a new one if (leafletMapRef.current) { leafletMapRef.current.remove(); mapSet.delete(leafletMapRef.current); } if (!mapElRef.current) return; + setIsMapReady(false); const map = L.map(mapElRef.current, opts || {}); initMap(map); + setIsMapReady(true); }, [geojson]); + useEffect(() => { + // After a Leaflet map is rendered, chache the map to reduce the cost for creating a map + if (isMapReady) { + const mapHTMLElements = document.getElementById(mapElId); + cachedLeafletMap.set(mapElId, mapHTMLElements?.outerHTML); + } + }, [isMapReady]); + /* If the geojson is different between renders, we need to recreate the map (happens because of FlashList's view recycling on the trip cards: https://shopify.github.io/flash-list/docs/recycling) */ - if (geoJsonIdRef.current && geoJsonIdRef.current !== geojson.data.id && leafletMapRef.current) { + if ( + !cachedLeafletMap.has(mapElId) && + geoJsonIdRef.current && + geoJsonIdRef.current !== geojson.data.id && + leafletMapRef.current + ) { leafletMapRef.current.eachLayer((layer) => leafletMapRef.current?.removeLayer(layer)); initMap(leafletMapRef.current); } - // non-alphanumeric characters are not safe for element IDs - const mapElId = `map-${geojson.data.id.replace(/[^a-zA-Z0-9]/g, '')}`; - return ( -
+ {cachedLeafletMap.has(mapElId) ? ( + parse(cachedLeafletMap.get(mapElId)) + ) : ( +
+ )} ); }; From 6f1f28c1d9f1e82418a431114b07d0fb1f8b8429 Mon Sep 17 00:00:00 2001 From: Jack Greenlee Date: Tue, 27 Feb 2024 22:25:39 -0500 Subject: [PATCH 05/14] downscale tiles on TripCard LeafletView Using lower res images for the tiles has a small performance benefit with scrolling. Since these are small maps, they do not need to be high-res. In fact, the text on the maps is larger and more readable with the lower res tiles, so this works out anyway. Note: The maps on LabelDetailsScreen still use high-res tiles because downscaleTiles is not set true there. --- www/js/components/LeafletView.tsx | 4 ++-- www/js/diary/cards/TripCard.tsx | 1 + 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/www/js/components/LeafletView.tsx b/www/js/components/LeafletView.tsx index f9854740c..07f2d9946 100644 --- a/www/js/components/LeafletView.tsx +++ b/www/js/components/LeafletView.tsx @@ -12,7 +12,7 @@ export function invalidateMaps() { mapSet.forEach((map) => map.invalidateSize()); } -const LeafletView = ({ geojson, opts, ...otherProps }) => { +const LeafletView = ({ geojson, opts, downscaleTiles, ...otherProps }: Props) => { const mapElRef = useRef(null); const leafletMapRef = useRef(null); const geoJsonIdRef = useRef(null); @@ -25,7 +25,7 @@ const LeafletView = ({ geojson, opts, ...otherProps }) => { L.tileLayer('https://tile.openstreetmap.org/{z}/{x}/{y}.png', { attribution: '© OpenStreetMap', opacity: 1, - detectRetina: true, + detectRetina: !downscaleTiles, }).addTo(map); const gj = L.geoJson(geojson.data, { pointToLayer: pointToLayer, diff --git a/www/js/diary/cards/TripCard.tsx b/www/js/diary/cards/TripCard.tsx index 43ea1aab7..b298c5453 100644 --- a/www/js/diary/cards/TripCard.tsx +++ b/www/js/diary/cards/TripCard.tsx @@ -116,6 +116,7 @@ const TripCard = ({ trip }: Props) => { Date: Tue, 27 Feb 2024 22:27:34 -0500 Subject: [PATCH 06/14] LeafletView: add Props type definition this component was made before we used TS in the project so the props weren't typed --- www/js/components/LeafletView.tsx | 11 ++++++++--- www/js/diary/cards/TripCard.tsx | 16 +++++++++------- 2 files changed, 17 insertions(+), 10 deletions(-) diff --git a/www/js/components/LeafletView.tsx b/www/js/components/LeafletView.tsx index 07f2d9946..c0c44f212 100644 --- a/www/js/components/LeafletView.tsx +++ b/www/js/components/LeafletView.tsx @@ -1,8 +1,8 @@ import React, { useEffect, useRef, useState } from 'react'; -import { View } from 'react-native'; +import { View, ViewProps } from 'react-native'; import { useTheme } from 'react-native-paper'; import L, { Map as LeafletMap } from 'leaflet'; -import { GeoJSONStyledFeature } from '../types/diaryTypes'; +import { GeoJSONData, GeoJSONStyledFeature } from '../types/diaryTypes'; import parse from 'html-react-parser'; const mapSet = new Set(); @@ -12,10 +12,15 @@ export function invalidateMaps() { mapSet.forEach((map) => map.invalidateSize()); } +type Props = ViewProps & { + geojson: GeoJSONData; + opts?: L.MapOptions; + downscaleTiles?: boolean; +}; const LeafletView = ({ geojson, opts, downscaleTiles, ...otherProps }: Props) => { const mapElRef = useRef(null); const leafletMapRef = useRef(null); - const geoJsonIdRef = useRef(null); + const geoJsonIdRef = useRef(null); const { colors } = useTheme(); // non-alphanumeric characters are not safe for element IDs const mapElId = `map-${geojson.data.id.replace(/[^a-zA-Z0-9]/g, '')}`; diff --git a/www/js/diary/cards/TripCard.tsx b/www/js/diary/cards/TripCard.tsx index b298c5453..2786975a8 100644 --- a/www/js/diary/cards/TripCard.tsx +++ b/www/js/diary/cards/TripCard.tsx @@ -113,14 +113,16 @@ const TripCard = ({ trip }: Props) => { {/* left panel */} - + style={[{ minHeight: windowWidth / 2 }, mapStyle]} + /> + )} {showAddNoteButton && ( From b81175a182439354122653ff727017ed4797ff4c Mon Sep 17 00:00:00 2001 From: Jack Greenlee Date: Tue, 27 Feb 2024 22:57:24 -0500 Subject: [PATCH 07/14] only show Leaflet + OSM attribution on the first trip Every trip on the Label screen has a map, and we've been showing the Leaflet and OSM attribution on every single one. This makes the DOM a little bit cluttered. Trimming it down could help performance marginally. Also, it's distracting and unnecessary to have it on every map. Leaflet does not care if you disable the attribution. But we will still attribute Leaflet because it's nice to do so :) The tiles from OSM are copyrighted; they DO care about attribution and have specific guidelines. In the guidelines they say "...if multiple static images appear on the same document, one instance of attribution is sufficient." https://osmfoundation.org/wiki/Licence/Attribution_Guidelines#Static_images These are static maps so it's good enough to show it on just one map. I think it's fair to put it on the most recent trip since that one will appear first in the list. This way, attribution will show on the first render but will not be distracting when scrolling up to other trips. Note that the full attribution always appears on the map on the LabelDetailsScreen, since that one is interactive and it's the only one on the page. So even on static maps where the attribution has been hidden, the attribution is 1 click away. --- www/js/diary/cards/TripCard.tsx | 6 +++--- www/js/diary/list/TimelineScrollList.tsx | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/www/js/diary/cards/TripCard.tsx b/www/js/diary/cards/TripCard.tsx index 2786975a8..ae8eaeddf 100644 --- a/www/js/diary/cards/TripCard.tsx +++ b/www/js/diary/cards/TripCard.tsx @@ -26,8 +26,8 @@ import { useGeojsonForTrip } from '../timelineHelper'; import { CompositeTrip } from '../../types/diaryTypes'; import { EnketoUserInputEntry } from '../../survey/enketo/enketoHelper'; -type Props = { trip: CompositeTrip }; -const TripCard = ({ trip }: Props) => { +type Props = { trip: CompositeTrip; isFirstInList?: boolean }; +const TripCard = ({ trip, isFirstInList }: Props) => { const { t } = useTranslation(); const { width: windowWidth } = useWindowDimensions(); const appConfig = useAppConfig(); @@ -54,7 +54,7 @@ const TripCard = ({ trip }: Props) => { navigation.navigate('label.details', { tripId, flavoredTheme }); } - const mapOpts = { zoomControl: false, dragging: false }; + const mapOpts = { attributionControl: isFirstInList, zoomControl: false, dragging: false }; const showAddNoteButton = appConfig?.survey_info?.buttons?.['trip-notes']; const mapStyle = showAddNoteButton ? s.shortenedMap : s.fullHeightMap; return ( diff --git a/www/js/diary/list/TimelineScrollList.tsx b/www/js/diary/list/TimelineScrollList.tsx index f705f56f8..dba47c49c 100644 --- a/www/js/diary/list/TimelineScrollList.tsx +++ b/www/js/diary/list/TimelineScrollList.tsx @@ -9,9 +9,9 @@ import LoadMoreButton from './LoadMoreButton'; import { useTranslation } from 'react-i18next'; import { Icon } from '../../components/Icon'; -const renderCard = ({ item: listEntry }) => { +const renderCard = ({ item: listEntry, index }) => { if (listEntry.origin_key.includes('trip')) { - return ; + return ; } else if (listEntry.origin_key.includes('place')) { return ; } else if (listEntry.origin_key.includes('untracked')) { From 915c63ce13a765174487c576ca9733b03d4c8f41 Mon Sep 17 00:00:00 2001 From: Jack Greenlee Date: Wed, 28 Feb 2024 16:26:19 -0500 Subject: [PATCH 08/14] set innerHTML directly instead of using html-react-parser We don't need this library because we can set innerHTML directly using dangerouslySetInnerHTML. This would be dangerous if we did not have control over where the html string came from. But we do have control; it is just a cached copy of something that was already rendered. So it is fine to use in this context. Also, the actual instance of the Leaflet map can be removed once its HTML is cached. --- www/js/components/LeafletView.tsx | 27 +++++++++++++++------------ 1 file changed, 15 insertions(+), 12 deletions(-) diff --git a/www/js/components/LeafletView.tsx b/www/js/components/LeafletView.tsx index c0c44f212..cc58aa3e9 100644 --- a/www/js/components/LeafletView.tsx +++ b/www/js/components/LeafletView.tsx @@ -3,7 +3,6 @@ import { View, ViewProps } from 'react-native'; import { useTheme } from 'react-native-paper'; import L, { Map as LeafletMap } from 'leaflet'; import { GeoJSONData, GeoJSONStyledFeature } from '../types/diaryTypes'; -import parse from 'html-react-parser'; const mapSet = new Set(); const cachedLeafletMap = new Map(); @@ -62,7 +61,8 @@ const LeafletView = ({ geojson, opts, downscaleTiles, ...otherProps }: Props) => // After a Leaflet map is rendered, chache the map to reduce the cost for creating a map if (isMapReady) { const mapHTMLElements = document.getElementById(mapElId); - cachedLeafletMap.set(mapElId, mapHTMLElements?.outerHTML); + cachedLeafletMap.set(mapElId, mapHTMLElements?.innerHTML); + leafletMapRef.current?.remove(); } }, [isMapReady]); @@ -122,16 +122,19 @@ const LeafletView = ({ geojson, opts, downscaleTiles, ...otherProps }: Props) => } } `} - {cachedLeafletMap.has(mapElId) ? ( - parse(cachedLeafletMap.get(mapElId)) - ) : ( -
- )} + +
); }; From cd67709552693ab8b8789994846a242257ed65bd Mon Sep 17 00:00:00 2001 From: Jack Greenlee Date: Wed, 28 Feb 2024 23:29:41 -0500 Subject: [PATCH 09/14] LeafletView: make the attribution hyperlinks functional To comply with OSM's attribution guidelines we need to link to the copyright page on their website. We do include links, but they haven't been working because elements don't work in our Cordova app like they would on the web. To open a URL we have to use cordova inappbrowser like this. I did the same for the Leaflet attribution as well. --- www/js/components/LeafletView.tsx | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/www/js/components/LeafletView.tsx b/www/js/components/LeafletView.tsx index cc58aa3e9..ff2458e89 100644 --- a/www/js/components/LeafletView.tsx +++ b/www/js/components/LeafletView.tsx @@ -7,6 +7,15 @@ import { GeoJSONData, GeoJSONStyledFeature } from '../types/diaryTypes'; const mapSet = new Set(); const cachedLeafletMap = new Map(); +// open the URL in the system browser & prevent any other effects of the click event +window['launchURL'] = (url, event) => { + window['cordova'].InAppBrowser.open(url, '_system'); + event.stopPropagation(); + return false; +}; +const osmURL = 'http://www.openstreetmap.org/copyright'; +const leafletURL = 'https://leafletjs.com'; + export function invalidateMaps() { mapSet.forEach((map) => map.invalidateSize()); } @@ -26,8 +35,11 @@ const LeafletView = ({ geojson, opts, downscaleTiles, ...otherProps }: Props) => const [isMapReady, setIsMapReady] = useState(false); function initMap(map: LeafletMap) { - L.tileLayer('https://tile.openstreetmap.org/{z}/{x}/{y}.png', { - attribution: '© OpenStreetMap', + map.attributionControl?.setPrefix( + `Leaflet`, + ); + const tileLayer = L.tileLayer('https://tile.openstreetmap.org/{z}/{x}/{y}.png', { + attribution: `© OpenStreetMap`, opacity: 1, detectRetina: !downscaleTiles, }).addTo(map); From a58b742c45434bb3590a5740a502414f22d8fe4e Mon Sep 17 00:00:00 2001 From: Jack Greenlee Date: Wed, 28 Feb 2024 23:41:15 -0500 Subject: [PATCH 10/14] LeafletView: only cache for TripCard maps The TripCard maps are being cached because we want to imporve performance on the trips list. But the maps on the label details screen should still be fully interactive; they should not be cached. Also, we can append '-downscaled' to the IDs of maps that use downscaled tiles; making them distinct from non-downscaled maps. this way Leaflet doesn't try to mix high-res and low-res tiles on the same map --- www/js/components/LeafletView.tsx | 45 +++++++++++++++++++------------ www/js/diary/cards/TripCard.tsx | 1 + 2 files changed, 29 insertions(+), 17 deletions(-) diff --git a/www/js/components/LeafletView.tsx b/www/js/components/LeafletView.tsx index ff2458e89..e3496b22e 100644 --- a/www/js/components/LeafletView.tsx +++ b/www/js/components/LeafletView.tsx @@ -1,4 +1,4 @@ -import React, { useEffect, useRef, useState } from 'react'; +import React, { useEffect, useMemo, useRef } from 'react'; import { View, ViewProps } from 'react-native'; import { useTheme } from 'react-native-paper'; import L, { Map as LeafletMap } from 'leaflet'; @@ -24,15 +24,22 @@ type Props = ViewProps & { geojson: GeoJSONData; opts?: L.MapOptions; downscaleTiles?: boolean; + cacheHtml?: boolean; }; -const LeafletView = ({ geojson, opts, downscaleTiles, ...otherProps }: Props) => { +const LeafletView = ({ geojson, opts, downscaleTiles, cacheHtml, ...otherProps }: Props) => { const mapElRef = useRef(null); const leafletMapRef = useRef(null); const geoJsonIdRef = useRef(null); const { colors } = useTheme(); - // non-alphanumeric characters are not safe for element IDs - const mapElId = `map-${geojson.data.id.replace(/[^a-zA-Z0-9]/g, '')}`; - const [isMapReady, setIsMapReady] = useState(false); + + // unique ID for map element, like "map-5f3e3b" or "map-5f3e3b-downscaled" + const mapElId = useMemo(() => { + let id = 'map-'; + // non-alphanumeric characters are not safe for element IDs + id += geojson.data.id.replace(/[^a-zA-Z0-9]/g, ''); + if (downscaleTiles) id += '-downscaled'; + return id; + }, [geojson.data.id, downscaleTiles]); function initMap(map: LeafletMap) { map.attributionControl?.setPrefix( @@ -52,6 +59,7 @@ const LeafletView = ({ geojson, opts, downscaleTiles, ...otherProps }: Props) => geoJsonIdRef.current = geojson.data.id; leafletMapRef.current = map; mapSet.add(map); + return tileLayer; } useEffect(() => { @@ -63,20 +71,18 @@ const LeafletView = ({ geojson, opts, downscaleTiles, ...otherProps }: Props) => mapSet.delete(leafletMapRef.current); } if (!mapElRef.current) return; - setIsMapReady(false); const map = L.map(mapElRef.current, opts || {}); - initMap(map); - setIsMapReady(true); - }, [geojson]); + const tileLayer = initMap(map); - useEffect(() => { - // After a Leaflet map is rendered, chache the map to reduce the cost for creating a map - if (isMapReady) { - const mapHTMLElements = document.getElementById(mapElId); - cachedLeafletMap.set(mapElId, mapHTMLElements?.innerHTML); - leafletMapRef.current?.remove(); + if (cacheHtml) { + new Promise((resolve) => tileLayer.on('load', resolve)).then(() => { + // After a Leaflet map is rendered, cache the map to reduce the cost for creating a map + const mapHTMLElements = document.getElementById(mapElId); + cachedLeafletMap.set(mapElId, mapHTMLElements?.innerHTML); + leafletMapRef.current?.remove(); + }); } - }, [isMapReady]); + }, [geojson, cacheHtml]); /* If the geojson is different between renders, we need to recreate the map (happens because of FlashList's view recycling on the trip cards: @@ -133,6 +139,9 @@ const LeafletView = ({ geojson, opts, downscaleTiles, ...otherProps }: Props) => /* glyph for 'flag' from https://pictogrammers.com/library/mdi/icon/flag/ */ '' } } + .leaflet-tile-loaded { + opacity: 1 !important; + } `}
dangerouslySetInnerHTML={ /* this is not 'dangerous' here because the content is not user-generated; it's just an HTML string that we cached from a previous render */ - cachedLeafletMap.has(mapElId) ? { __html: cachedLeafletMap.get(mapElId) } : undefined + cacheHtml && cachedLeafletMap.has(mapElId) + ? { __html: cachedLeafletMap.get(mapElId) } + : undefined } /> diff --git a/www/js/diary/cards/TripCard.tsx b/www/js/diary/cards/TripCard.tsx index ae8eaeddf..3504bde16 100644 --- a/www/js/diary/cards/TripCard.tsx +++ b/www/js/diary/cards/TripCard.tsx @@ -118,6 +118,7 @@ const TripCard = ({ trip, isFirstInList }: Props) => { geojson={tripGeojson} opts={mapOpts} downscaleTiles={true} + cacheHtml={true} /* the map should be at least as tall as it is wide so it doesn't look squished */ style={[{ minHeight: windowWidth / 2 }, mapStyle]} From 10ca74154c506b78d71ff591bf717f5f5ffd74a4 Mon Sep 17 00:00:00 2001 From: Jack Greenlee Date: Thu, 29 Feb 2024 00:07:31 -0500 Subject: [PATCH 11/14] use reversed FlatList without CSS transforms https://github.com/e-mission/e-mission-phone/pull/1132 We are trying to improve performance of the timeline scroll. The way that FlashList / FlatList 'inverted' property works has shown to be a bottleneck. It "uses CSS transforms to flip the entire list and then flip each list item back, which is a performance hit and causes scrolling to be choppy, especially on old iPhones." An alternative I found is setting the 'flex-direction' to 'column-reverse'. But this only works for FlatList. 'column-reverse' is applied to the content so the items in the list display bottom-to-top, and also applied to the wrapper so the scroll position initializes from the bottom. This yields better performance! But there is another issue: on iOS the list flashes for a second while loaded and then goes blank. Only once the user interacts is the list visible. I think this is an issue with iOS Safari. I came up with a workaround to add a temporary 1px margin to trigger a layout update. It's not ideal, but it's minimally intrusive and won't cause further problems. Eventually if we fully migrate to React Native, we will not have to worry about Safari at all; we should be able to just use FlatList or FlashList as-is. --- www/js/diary/list/TimelineScrollList.tsx | 47 ++++++++++++------------ 1 file changed, 24 insertions(+), 23 deletions(-) diff --git a/www/js/diary/list/TimelineScrollList.tsx b/www/js/diary/list/TimelineScrollList.tsx index dba47c49c..9f297fa2c 100644 --- a/www/js/diary/list/TimelineScrollList.tsx +++ b/www/js/diary/list/TimelineScrollList.tsx @@ -1,9 +1,8 @@ import React from 'react'; -import { FlashList } from '@shopify/flash-list'; import TripCard from '../cards/TripCard'; import PlaceCard from '../cards/PlaceCard'; import UntrackedTimeCard from '../cards/UntrackedTimeCard'; -import { View } from 'react-native'; +import { View, FlatList } from 'react-native'; import { ActivityIndicator, Banner, Text } from 'react-native-paper'; import LoadMoreButton from './LoadMoreButton'; import { useTranslation } from 'react-i18next'; @@ -40,6 +39,7 @@ const TimelineScrollList = ({ isLoading, }: Props) => { const { t } = useTranslation(); + const listRef = React.useRef(null); // The way that FlashList inverts the scroll view means we have to reverse the order of items too const reversedListEntries = listEntries ? [...listEntries].reverse() : []; @@ -82,11 +82,11 @@ const TimelineScrollList = ({ } else if (listEntries) { /* Condition: we've successfully loaded and set `listEntries`, so show the list */ return ( - item._id.$oid} /* TODO: We can capture onScroll events like this, so we should be able to automatically load more trips when the user is approaching the bottom or top of the list. @@ -97,24 +97,25 @@ const TimelineScrollList = ({ } ListFooterComponent={isLoading == 'prepend' ? smallSpinner : footer} ItemSeparatorComponent={separator} - /* Percent of viewport that must be covered for a partially occluded item to count as "viewable", 0-100. - a single pixel in the viewport makes the item viewable */ - viewabilityConfig={{ - viewAreaCoveragePercentThreshold: 0, - itemVisiblePercentThreshold: 0, - waitForInteraction: false, - }} - // 'getItemType' prop is used when FlashList has different types of cell components and these are vastly different - getItemType={(item) => { - if (item.origin_key.includes('trip')) { - return 'trip'; - } else if (item.origin_key.includes('place')) { - return 'place'; - } else if (item.origin_key.includes('untracked')) { - return 'untracked'; - } else { - return 'unknown'; - } + /* use column-reverse so that the list is 'inverted', meaning it should start + scrolling from the bottom, and the bottom-most item should be first in the DOM tree + This method is used instead of the `inverted` property of FlatList, because `inverted` + uses CSS transforms to flip the entire list and then flip each list item back, which + is a performance hit and causes scrolling to be choppy, especially on old iPhones. */ + style={{ flexDirection: 'column-reverse' }} + contentContainerStyle={{ flexDirection: 'column-reverse' }} + /* Workaround for iOS Safari bug where a 'column-reverse' element containing scroll content + shows up blank until it's scrolled or its layout changes. + Adding a temporary 1px margin-right, and then removing it on the next event loop, + is the least intrusive way I've found to trigger a layout change. + It basically just jiggles the element so it doesn't blank out. */ + onContentSizeChange={() => { + console.debug('TimelineScrollList onContentSizeChange'); + const list = document.getElementById('timelineScrollList'); + list?.style.setProperty('margin-right', '1px'); + setTimeout(() => { + list?.style.setProperty('margin-right', '0'); + }); }} /> ); From 08481315d1f12520e92744128ee3f600194af53e Mon Sep 17 00:00:00 2001 From: Sebastian Barry <61334340+sebastianbarry@users.noreply.github.com> Date: Thu, 29 Feb 2024 10:37:11 -0700 Subject: [PATCH 12/14] Moved raw_data_use backwards compat hack Moved backwards compat hack to fill in the raw_data_use for programs that don't have it to PrivacyPolicy.tsx, from ProfileSettings.tsx so that the privacy policy is updated for the login screen, and it carries over to the Profile settings screen. --- www/js/control/ProfileSettings.tsx | 9 --------- www/js/onboarding/PrivacyPolicy.tsx | 11 +++++++++++ 2 files changed, 11 insertions(+), 9 deletions(-) diff --git a/www/js/control/ProfileSettings.tsx b/www/js/control/ProfileSettings.tsx index 4a33a6a68..9a639dd20 100644 --- a/www/js/control/ProfileSettings.tsx +++ b/www/js/control/ProfileSettings.tsx @@ -131,15 +131,6 @@ const ProfileSettings = () => { function whenReady(newAppConfig: AppConfig) { const tempUiConfig = newAppConfig; - // backwards compat hack to fill in the raw_data_use for programs that don't have it - const default_raw_data_use = { - en: `to monitor the ${tempUiConfig.intro.program_or_study}, send personalized surveys or provide recommendations to participants`, - es: `para monitorear el ${tempUiConfig.intro.program_or_study}, enviar encuestas personalizadas o proporcionar recomendaciones a los participantes`, - }; - Object.entries(tempUiConfig.intro.translated_text).forEach(([lang, val]) => { - val.raw_data_use = val.raw_data_use || default_raw_data_use[lang]; - }); - // Backwards compat hack to fill in the `app_required` based on the // old-style "program_or_study" // remove this at the end of 2023 when all programs have been migrated over diff --git a/www/js/onboarding/PrivacyPolicy.tsx b/www/js/onboarding/PrivacyPolicy.tsx index 8287595c9..8fe12961d 100644 --- a/www/js/onboarding/PrivacyPolicy.tsx +++ b/www/js/onboarding/PrivacyPolicy.tsx @@ -36,6 +36,17 @@ const PrivacyPolicy = () => { ); } + // backwards compat hack to fill in the raw_data_use for programs that don't have it + if (appConfig?.intro) { + const default_raw_data_use = { + en: `monitor the ${appConfig?.intro?.program_or_study}, send personalized surveys or provide recommendations to participants`, + es: `monitorear el ${appConfig?.intro?.program_or_study}, enviar encuestas personalizadas o proporcionar recomendaciones a los participantes`, + }; + Object.entries(appConfig?.intro?.translated_text).forEach(([lang, val]: [string, any]) => { + val.raw_data_use = val.raw_data_use || default_raw_data_use[lang]; + }); + } + const templateText = useMemo(() => getTemplateText(appConfig, i18n.language), [appConfig]); return ( From 16b114b36575abc24f2c4e64bae725438bb8070d Mon Sep 17 00:00:00 2001 From: Jack Greenlee Date: Thu, 29 Feb 2024 12:38:57 -0500 Subject: [PATCH 13/14] move leaflet cache to a stateful hook There was an issue where map elements were sometimes showing blank when they should have been cached. This is because they were stored in a Map object, in LeafletView.tsx but outside of the component. It was not kept as state so React didn't know to re-render if it changed. We can keep this Map object as state in a custom hook that all LeafletViews can use. --- www/js/components/LeafletView.tsx | 15 +++++++-------- www/js/components/useLeafletCache.ts | 10 ++++++++++ 2 files changed, 17 insertions(+), 8 deletions(-) create mode 100644 www/js/components/useLeafletCache.ts diff --git a/www/js/components/LeafletView.tsx b/www/js/components/LeafletView.tsx index e3496b22e..b5d6b8dbe 100644 --- a/www/js/components/LeafletView.tsx +++ b/www/js/components/LeafletView.tsx @@ -3,9 +3,9 @@ import { View, ViewProps } from 'react-native'; import { useTheme } from 'react-native-paper'; import L, { Map as LeafletMap } from 'leaflet'; import { GeoJSONData, GeoJSONStyledFeature } from '../types/diaryTypes'; +import useLeafletCache from './useLeafletCache'; const mapSet = new Set(); -const cachedLeafletMap = new Map(); // open the URL in the system browser & prevent any other effects of the click event window['launchURL'] = (url, event) => { @@ -31,6 +31,7 @@ const LeafletView = ({ geojson, opts, downscaleTiles, cacheHtml, ...otherProps } const leafletMapRef = useRef(null); const geoJsonIdRef = useRef(null); const { colors } = useTheme(); + const leafletCache = useLeafletCache(); // unique ID for map element, like "map-5f3e3b" or "map-5f3e3b-downscaled" const mapElId = useMemo(() => { @@ -63,8 +64,8 @@ const LeafletView = ({ geojson, opts, downscaleTiles, cacheHtml, ...otherProps } } useEffect(() => { - // if a Leaflet map is chached, there is no need to create the map again - if (cachedLeafletMap.has(mapElId)) return; + // if a Leaflet map is cached, there is no need to create the map again + if (cacheHtml && leafletCache.has(mapElId)) return; // if a Leaflet map already exists (because we are re-rendering), remove it before creating a new one if (leafletMapRef.current) { leafletMapRef.current.remove(); @@ -78,7 +79,7 @@ const LeafletView = ({ geojson, opts, downscaleTiles, cacheHtml, ...otherProps } new Promise((resolve) => tileLayer.on('load', resolve)).then(() => { // After a Leaflet map is rendered, cache the map to reduce the cost for creating a map const mapHTMLElements = document.getElementById(mapElId); - cachedLeafletMap.set(mapElId, mapHTMLElements?.innerHTML); + leafletCache.set(mapElId, mapHTMLElements?.innerHTML); leafletMapRef.current?.remove(); }); } @@ -88,7 +89,7 @@ const LeafletView = ({ geojson, opts, downscaleTiles, cacheHtml, ...otherProps } (happens because of FlashList's view recycling on the trip cards: https://shopify.github.io/flash-list/docs/recycling) */ if ( - !cachedLeafletMap.has(mapElId) && + !leafletCache.has(mapElId) && geoJsonIdRef.current && geoJsonIdRef.current !== geojson.data.id && leafletMapRef.current @@ -153,9 +154,7 @@ const LeafletView = ({ geojson, opts, downscaleTiles, cacheHtml, ...otherProps } dangerouslySetInnerHTML={ /* this is not 'dangerous' here because the content is not user-generated; it's just an HTML string that we cached from a previous render */ - cacheHtml && cachedLeafletMap.has(mapElId) - ? { __html: cachedLeafletMap.get(mapElId) } - : undefined + cacheHtml && leafletCache.has(mapElId) ? { __html: leafletCache.get(mapElId) } : undefined } /> diff --git a/www/js/components/useLeafletCache.ts b/www/js/components/useLeafletCache.ts new file mode 100644 index 000000000..9c6037c3f --- /dev/null +++ b/www/js/components/useLeafletCache.ts @@ -0,0 +1,10 @@ +import { useState } from 'react'; +export default function useLeafletCache() { + const [cachedMaps, setCachedMaps] = useState(new Map()); + + return { + has: (key: string) => cachedMaps.has(key), + get: (key: string) => cachedMaps.get(key), + set: (key: string, value: any) => setCachedMaps((prev) => new Map(prev.set(key, value))), + }; +} From 8b5c6f6d141b8160701d76248d3cef05ebd9d86f Mon Sep 17 00:00:00 2001 From: Jijeong Lee Date: Sun, 3 Mar 2024 20:00:59 -1000 Subject: [PATCH 14/14] delete unnecessary print --- www/js/diary/list/TimelineScrollList.tsx | 1 - 1 file changed, 1 deletion(-) diff --git a/www/js/diary/list/TimelineScrollList.tsx b/www/js/diary/list/TimelineScrollList.tsx index 9f297fa2c..2932f6ae0 100644 --- a/www/js/diary/list/TimelineScrollList.tsx +++ b/www/js/diary/list/TimelineScrollList.tsx @@ -110,7 +110,6 @@ const TimelineScrollList = ({ is the least intrusive way I've found to trigger a layout change. It basically just jiggles the element so it doesn't blank out. */ onContentSizeChange={() => { - console.debug('TimelineScrollList onContentSizeChange'); const list = document.getElementById('timelineScrollList'); list?.style.setProperty('margin-right', '1px'); setTimeout(() => {