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

ContentSearchResult #747

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

ContentSearchResult #747

wants to merge 2 commits into from

Conversation

apepper
Copy link
Contributor

@apepper apepper commented Feb 10, 2025

Moves search logic of https://github.com/Scrivito/scrivito-portal-app/blob/main/src/Widgets/SearchResultsWidget/SearchResultsWidgetComponent.tsx into a data class. Overall goal: Get rid of SearchResultsWidget and use data search primitives.

Working copy to test it out: https://edit.scrivito.com/d0a154d76edf2a7bd991fc658e700a1d~https://contentsearchresult.scrivito-portal-app.pages.dev/en/search-results?q=Test&_scrivito_workspace_id=r228d708e9d6524d&_scrivito_display_mode=view

Please not that other PRs are still needed before the content can go live.

@apepper apepper requested a review from dcsaszar February 10, 2025 13:40
@dcsaszar dcsaszar self-assigned this Feb 12, 2025
Copy link
Contributor

@dcsaszar dcsaszar left a comment

Choose a reason for hiding this comment

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

Cool. It's obvious that we lose the live updating by this approach but I guess there currently is no way around it. An invalidation mechanism would be nice to have 😜 .

See minor feedback in comments.

'reference',
{ title: lang === 'de' ? 'Bild' : 'Image', to: 'Image' },
],
snippet: ['string', { title: lang === 'de' ? 'Schnipsel' : 'Snippet' }],
Copy link
Contributor

Choose a reason for hiding this comment

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

  •  Maybe more formal "Textausschnitt"?

},
title: async () =>
(await load(currentLanguage)) === 'de'
? 'Inhalts-Suchergebnis'
Copy link
Contributor

Choose a reason for hiding this comment

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

  • FYI To make this more readable maybe it's sufficient to have just "search results" and (later on) "data search results". Alternatively maybe sth like "Suchtreffer (Content)"?

connection: {
async index(params) {
if (Object.keys(params.filters()).length > 0) {
throw new Error('Filtering is not supported for CMS Search Results.')
Copy link
Contributor

Choose a reason for hiding this comment

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

  • FYI strange capitalization in "Search Results".


const objSearch = Obj.whereFullTextOf('*', 'matches', search)
.andNot('_objClass', 'equals', BLACKLIST_OBJ_CLASSES)
.and('_dataParam', 'equals', null) // Ignore data details pages
Copy link
Contributor

Choose a reason for hiding this comment

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

  • Undocumented feature. Is there an issue for that? Maybe add a bit more about how/why/for how long this works to the comment? How about removing this magic and setting excludeFromSearch: true for these pages?

Comment on lines +72 to +74
const snippet =
extractText(obj, { length: 300 }) ||
ensureString(obj.get('metaDataDescription'))
Copy link
Contributor

Choose a reason for hiding this comment

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

  •  FMI how often/for which pages/objects is the fallback code path taken?


function objToResult(obj: Obj) {
const imageReference = obj.get('image')
const image = isImage(imageReference) ? imageReference.id() : null
Copy link
Contributor

Choose a reason for hiding this comment

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

  •  FYI maybe use imageId here for more clarity about the data in question.

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