Skip to content

Catalog search provider#7834

Open
zoran995 wants to merge 2 commits intoupdate-dev-gulp-scriptfrom
catalog-search-provider
Open

Catalog search provider#7834
zoran995 wants to merge 2 commits intoupdate-dev-gulp-scriptfrom
catalog-search-provider

Conversation

@zoran995
Copy link
Copy Markdown
Collaborator

What this PR does

Fixes #

Move the catalogue search provider from SearchBarModel to the Catalog class. I always felt that the catalogue search provider doesn't belong in SearchBarModel, but at the time I was writing it, I wasn't aware of Catalog, so it stayed in the model.

Test me

How should reviewers test this? (Hint: If you want to provide a test catalog item, create a Gist of its catalog JSON, add its url and your branch name to this url: http://ci.terria.io/<branch name>/#clean&<raw url of your gist> , and then paste that in the body of this PR)

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 relevant documentation in doc/.
  • I've updated CHANGES.md with what I changed.
  • I've provided instructions in the PR description on how to test this PR.

@zoran995 zoran995 requested review from ljowen and na9da April 23, 2026 07:22
Copy link
Copy Markdown
Collaborator

@na9da na9da left a comment

Choose a reason for hiding this comment

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

Hi @zoran995 - looks great.

One small thing I noticed is we do a cast here to access a property not part of the interface:

debounceDuration={
terria.catalogReferencesLoaded
? (
searchState.catalogSearchProvider as CatalogSearchProvider
).debounceDurationOnceLoaded
: DEBOUNCE_INTERVAL

We should maybe make it part of the interface or traits instead.

Comment on lines +32 to +36
new CatalogSearchProvider(
"catalog-search-provider",
terria,
terria.searchBarModel.minCharacters
);
Copy link
Copy Markdown
Collaborator

@na9da na9da Apr 24, 2026

Choose a reason for hiding this comment

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

Suggestion: I know we have done this elsewhere in the past but it might be a good idea to stick with standard Constructor parameters for models? Using custom constructor parameters is fine if the model is always initialised manually but if at some point we do automatic initialisation from some config, it could create problems.

One other pattern to make creating the instance easier would be to define a static method like CatalogSearchProvider.fromOptions({minCharacters})

Comment on lines +18 to +19
searchProvider: CatalogSearchProviderMixin.Instance | undefined;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Can this be undefined if always intialised in constructor?

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.

2 participants