Skip to content
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.

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

Fix blank screen issue when scrolling on trip list #1132

Merged
merged 11 commits into from
Mar 4, 2024

Commits on Feb 22, 2024

  1. Configuration menu
    Copy the full SHA
    6f101d1 View commit details
    Browse the repository at this point in the history

Commits on Feb 26, 2024

  1. Configuration menu
    Copy the full SHA
    f9115ab View commit details
    Browse the repository at this point in the history

Commits on Feb 28, 2024

  1. 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.
    JGreenlee committed Feb 28, 2024
    Configuration menu
    Copy the full SHA
    6f1f28c View commit details
    Browse the repository at this point in the history
  2. LeafletView: add Props type definition

    this component was made before we used TS in the project so the props weren't typed
    JGreenlee committed Feb 28, 2024
    Configuration menu
    Copy the full SHA
    187dfb3 View commit details
    Browse the repository at this point in the history
  3. 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.
    JGreenlee committed Feb 28, 2024
    Configuration menu
    Copy the full SHA
    b81175a View commit details
    Browse the repository at this point in the history
  4. 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.
    JGreenlee committed Feb 28, 2024
    Configuration menu
    Copy the full SHA
    915c63c View commit details
    Browse the repository at this point in the history

Commits on Feb 29, 2024

  1. 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 <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.
    JGreenlee committed Feb 29, 2024
    Configuration menu
    Copy the full SHA
    cd67709 View commit details
    Browse the repository at this point in the history
  2. 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
    JGreenlee committed Feb 29, 2024
    Configuration menu
    Copy the full SHA
    a58b742 View commit details
    Browse the repository at this point in the history
  3. use reversed FlatList without CSS transforms

    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.
    JGreenlee committed Feb 29, 2024
    Configuration menu
    Copy the full SHA
    10ca741 View commit details
    Browse the repository at this point in the history
  4. 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.
    JGreenlee committed Feb 29, 2024
    Configuration menu
    Copy the full SHA
    16b114b View commit details
    Browse the repository at this point in the history

Commits on Mar 4, 2024

  1. delete unnecessary print

    jiji14 committed Mar 4, 2024
    Configuration menu
    Copy the full SHA
    8b5c6f6 View commit details
    Browse the repository at this point in the history