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

Configurable search providers from config #5145

Merged
merged 77 commits into from
Nov 23, 2023

Conversation

zoran995
Copy link
Contributor

@zoran995 zoran995 commented Jan 19, 2021

What this PR does

Fixes #5141
Requires TerriaJS/TerriaMap#515

After investigation as the best option appears to utilize the existing model system for SearchProviders.

  • wrote proposal as architecture decision record
  • added SearchProviderMixin to connect searchProviders with a model system
  • ported existing search providers to the proposed model
  • added defaults to BingMapsSearchProviders and AustralianGazetteer for testing purposes.

Testing

Default SearchProviders are BingMaps and AustralianGazetteer, need to apply registerSearchProviders and comment out reference to BingMapsSearchProviderViewModel.

Pending

  • Make everything support i18next code and load correct names in GUI
  • Tests
  • Documentation
  • Create TerriaMap PR

Future work

  • Introduce server-side WFS SearchProvider which will perform a search using terriajs-server and return the results.

Checklist

  • There are unit tests to verify my changes are correct or unit tests aren't applicable (if so, write quick reason why unit tests don't exist)
  • I've updated CHANGES.md with what I changed.

- add SearchProviderMixin to connect searchProviders with model system
- write architecture decission record
- port existing search providers to proposed model
@zoran995 zoran995 marked this pull request as draft January 19, 2021 13:51
lib/Models/Terria.ts Outdated Show resolved Hide resolved
@zoran995 zoran995 marked this pull request as ready for review February 18, 2021 00:08
@zoran995
Copy link
Contributor Author

This is ready for review, if anyone has time to check.
p.s. Sorry, it turned out to be a monster PR.

@zoran995
Copy link
Contributor Author

zoran995 commented Nov 7, 2023

Hi @nf-s I have synced the code with the latest main and it is ready for review.
p.s. Would you like me to completely rebase and squash the code in a single commit to avoid polluting history with 2+ years old commits?

@nf-s nf-s assigned ljowen and nf-s and unassigned tephenavies Nov 8, 2023
@nf-s
Copy link
Contributor

nf-s commented Nov 8, 2023

Hi @zoran995
Thanks so much!
A rebase would be great. We don't think squash is required - but whatever you prefer!

We will try to review it in the next few weeks.

Thanks again 🙂

@nf-s nf-s mentioned this pull request Nov 15, 2023

this._catalogSearchDisposer = reaction(
() => this.catalogSearchText,
() => self.catalogSearchText,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't believe this is needed since arrow functions use the enclosing this value

Copy link
Contributor

@ljowen ljowen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Tested locally with Bing and Cesium search providers. Couple of small comments

doc/customizing/search-providers.md Show resolved Hide resolved
lib/Models/SearchProviders/CesiumIonSearchProvider.ts Outdated Show resolved Hide resolved
test/Models/SearchProviders/BingMapsSearchProviderSpec.ts Outdated Show resolved Hide resolved
@ljowen ljowen merged commit c8639e3 into TerriaJS:main Nov 23, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Search bar configurable from config
5 participants