Label scroll optimizations #1134
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
this component was made before we used TS in the project so the props weren't typed
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.
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.
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 <a> 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.
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
e-mission#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.
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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.