Skip to content

Add OpenSearch description#336

Open
starkmarkus wants to merge 9 commits into
NuschtOS:mainfrom
starkmarkus:add-opensearch-description
Open

Add OpenSearch description#336
starkmarkus wants to merge 9 commits into
NuschtOS:mainfrom
starkmarkus:add-opensearch-description

Conversation

@starkmarkus

Copy link
Copy Markdown

Adds an OpenSearch description file and links it from the document head so browsers can discover the search endpoint more easily. Addresses #154.

@MarcelCoding

Copy link
Copy Markdown
Member

I am wondering whether we should add a separate one for the packages and options search part?

@MarcelCoding

Copy link
Copy Markdown
Member

Also the endpoint probably should be on /packages or /options and not just on /

@MarcelCoding

Copy link
Copy Markdown
Member

Also the actual title would be included.

@starkmarkus

Copy link
Copy Markdown
Author

Good point. I agree the OpenSearch entry should likely be split so packages and options can have their own targets instead of one generic root link. I will adjust the implementation in that direction unless you prefer a different shape.

Signed-off-by: Markus Stark <markusstark@MacBook-Air-von-Markus.local>
@starkmarkus

Copy link
Copy Markdown
Author

Implemented the OpenSearch update: the PR now exposes separate OpenSearch descriptions for packages and options instead of one generic description.

@MarcelCoding

Copy link
Copy Markdown
Member

Don't we manually want to include the opensearch.xml of the current search type dynamically. (options vs packages.)

Also this is till open: I mean the title from the config

Also the actual title would be included.

@starkmarkus

Copy link
Copy Markdown
Author

@MarcelCoding Implemented the follow-up: the OpenSearch link is now selected dynamically for the active search type (options vs. packages), and the OpenSearch title is now taken from the build config instead of being hard-coded.

@MarcelCoding

Copy link
Copy Markdown
Member

I would move it into the comments that are rendered by the router. Then we don't need to handle the router events ourself.

@MarcelCoding MarcelCoding left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

But let's wait for the preview deployment.

@MarcelCoding

Copy link
Copy Markdown
Member

The compiler is not happy yet.

@starkmarkus

Copy link
Copy Markdown
Author

Fixed the TS4111 compiler issue by switching the dataset access to bracket notation. The branch is pushed again, so CI should rerun on the updated commit.

Comment thread nix/frontend.nix
@MarcelCoding

Copy link
Copy Markdown
Member

Did you manage to test this behavior locally? If I am on GitHub and type something in the URL bar in Firefox, there is an entry "search with github". If I to the same with a local version of the search, it's missing.

@starkmarkus

Copy link
Copy Markdown
Author

I checked this locally just now: the OpenSearch link is correctly injected into the page head on both routes. /options uses opensearch-options.xml, and /packages uses opensearch-packages.xml. I had to fix a local startup issue first because @angular/cdk was missing from the dependencies, but after that the browser entry showed up as expected.

@starkmarkus

Copy link
Copy Markdown
Author

@MarcelCoding I also pushed a small follow-up for the failing Pages workflow on this branch. The parse error came from using secrets inside an if: expression; the deploy step now only runs on push, so PR runs should no longer fail on that workflow definition.

@MarcelCoding

Copy link
Copy Markdown
Member

I checked this locally just now: the OpenSearch link is correctly injected into the page head on both routes. /options uses opensearch-options.xml

That already was it from the beginning. But the browser does not interpret it for me. Maybe its an issue with localhost or, because its inserted using javascript and being present from the beginning in the index.html?

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.

3 participants