OpenHistoricalMap / issues

File your issues here, regardless of repo until we get all our repos squared away; we don't want to miss anything.
Creative Commons Zero v1.0 Universal
19 stars 1 forks source link

Update timeslider to support expression-based stylesheets #855

Open jeffreyameyer opened 3 months ago

jeffreyameyer commented 3 months ago

Bug & Feature description Now that our stylesheets have been upgraded to expressions (#775), our timeslider needs to be updated to support this change. I believe the old timeslider was based on filters that are no longer available. (Staging was down, so I could not verify at this time)

In addition, while doing this work, we may want to move to supporting a library for time filtering such as the one described in #840.

1ec5 commented 3 months ago

https://github.com/gramps-project/gramps-web/pull/454#discussion_r1664621357 illustrates the changes that would need to take place. Unfortunately, the leaflet-ohm-timeslider-v2 code is much more complex than all the other time filtering implementations, so it might be more difficult to modify it to support expressions. My guess is that it would go somewhere in here:

https://github.com/OpenHistoricalMap/leaflet-ohm-timeslider-v2/blob/73e14c116aaa474e30cec2eec33cd0bc8c56a062/leaflet-ohm-timeslider.js#L645-L735

If we publish a dedicated library for time filtering, I’d suggest starting with one of the simpler implementations and adding special cases (if any) for parity with leaflet-ohm-timeslider-v2.

danrademacher commented 3 months ago

This might seem like a silly question, but: How do we know that the timeslider in fact needs to be refactored?

I asked Gregor about this a few days ago, and his conclusion was that the timeslider was already using expressions to filter, regardless of the fact that the underlying stylesheet was not using expressions to style.

As a way to verify this, I updated the static map style in the Leaflet timeslider demo with the latest version of main.json,, and found that it seems to work fine.

That demo is here: https://openhistoricalmap.github.io/leaflet-ohm-timeslider-v2/

I realized there are other desires at play here, such as moving away from Leaflet and modularizing the method of filtering years, but in terms of a blocker to multilingual labels, I don't see a pressing need to refactor the timeslider.

jeffreyameyer commented 3 months ago

I believe this was thought to be the case, but that thought could be wrong.

This should be quickly verifiable: If the expression-based stylesheet has been deployed to staging, the timeslider there shouldn't work.

I believe that the expression-based stylesheet has been deployed to staging and by my quick check, the timeslider is working.

@1ec5 - do you know if there was something else that might trigger errors or if there are some edge cases we might want to doublecheck?

1ec5 commented 3 months ago

That’s because the style’s filters haven’t been converted to use expressions yet. For example, this filter on the roads_residential layer is using the legacy syntax:

[
  "all",
  ["in", "type", "residential", "service", "unclassified"]
]

The equivalent expression would be:

[
  "all",
  ["in", ["get", "type"], ["residential", "service", "unclassified"]]
]

This code would combine legacy filter syntax into whatever the style layer currently has as a filter. But mixing the legacy filter syntax and an expression-based filter into a single filter is guaranteed to trigger an exception. So yes, I do think the time slider will break once any of the filters is upgraded to expressions.

1ec5 commented 2 months ago

In addition, while doing this work, we may want to move to supporting a library for time filtering such as the one described in https://github.com/OpenHistoricalMap/issues/issues/840.

maplibre-gl-dates now supports both the legacy filter syntax and expression-based filters. Once we publish it to NPM, we’ll be able to migrate both ohm-embed-1yr and leaflet-ohm-timeslider-v2 to use it. However, neither project currently uses NPM, so we’d need to rely on a copy of the plugin vendored into ohm-website and hosted on openhistoricalmap.org. It shouldn’t be too difficult to migrate ohm-embed-1yr to NPM. If we can also migrate leaflet-ohm-timeslider-v2 to NPM, it would be much more reusable by other projects.