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

[WIP] 10502 search layer #11626

Draft
wants to merge 9 commits into
base: dev/8.0.x
Choose a base branch
from
Draft

[WIP] 10502 search layer #11626

wants to merge 9 commits into from

Conversation

whatisgalen
Copy link
Member

@whatisgalen whatisgalen commented Nov 15, 2024

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Description of Change

dependencies: #11629 #11628

Issues Solved

Closes #10502

Checklist

  • I targeted one of these branches:
    • dev/8.0.x (under development): features, bugfixes not covered below
    • dev/7.6.x (main support): regressions, crashing bugs, security issues, major bugs in new features
    • dev/6.2.x (extended support): major security issues, data loss issues
  • I added a changelog in arches/releases
  • I submitted a PR to arches-docs (if appropriate)
  • Unit tests pass locally with my changes
  • I added tests that prove my fix is effective or that my feature works
  • My test fails on the target branch

Accessibility Checklist

Developer Guide

Topic Changed Retested
Color contrast
Form fields
Headings
Links
Keyboard
Responsive Design
HTML validation
Screen reader

Ticket Background

  • Sponsored by:
  • Found by: @
  • Tested by: @
  • Designed by: @

Further comments

  • caching
  • ask the server to do computation, not end user machine

@whatisgalen whatisgalen marked this pull request as draft November 15, 2024 21:00
@whatisgalen whatisgalen changed the title 10502 search layer [WIP] 10502 search layer Nov 18, 2024
@robgaston
Copy link
Member

robgaston commented Nov 26, 2024

Some off-the-cuff thoughts on a quick read of this code:

  • the basic pattern here holds water I think but will likely need work in several areas
  • am I reading properly that GeoTileGridAgg is being used to achieve geospatial clusters? This is problematic as an approach to clustering for a number of reasons:
    • it doesn't seek to group nearby items when they are nearly coincident, but rather simply bins geometries into predefined cells (for example, two very spatially close records might not be clustered together if they are separated by a cell boundary).
    • Additionally, clusters should truly be generated across tile boundaries for much the same reasons (tiles are arbitrary pre-defined cells which may separate very spatially close geometries), which a tile based aggregation will never account for.
    • Grid cell centroids are almost always not the best proximate location for a clustered set of geometries
  • I would argue that the service class, route, etc. should not bear the name "search_layer" as layers are a presentation construct and these are defining data services. The names here should be more descriptive of the API/response.
  • In my opinion there should not be a pre-defined layer in the map JS viewmodel and instead it should just set up the data source which can then be used in overlays freely. This allows search layers to be defined, customized, and controlled by the end user as we do all other overlays.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Discussion: Unpaginated Search Results and Supporting a Dynamic Search Results Map Layer
2 participants