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.

Sign up for GitHub

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

Jump to bottom

feat: new search #2296

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

feat: new search #2296

wants to merge 2 commits into from

Conversation

Jonas-C
Copy link
Contributor

@Jonas-C Jonas-C commented Jan 6, 2025 •
edited
Loading

TODO:

  • Loading-skeleton
  • Loading-indikator for pågående søk
  • Suggestions
  • Finne ut av hvordan ressurstype-filter skal funke
  • Drøfte fag i vanlig søk
  • Fjerne gammel kode :D
  • Rename ting som avventer sletting av gammel kode
  • Nynorsk-sjekk av oversettelser
  • Flytte LTI over på ny søkeside
  • Flytte fokus til toppen ved paginering
  • Penere subject-parametre i URL (droppe urn:subject:)

@Jonas-C Jonas-C force-pushed the feat/new-search branch 4 times, most recently from 4eb499a to cd24b1c Compare February 12, 2025 12:10
gunnarvelle
gunnarvelle reviewed Feb 13, 2025
View reviewed changes
@gunnarvelle
Copy link
Member

Første pagineringsklikk på Neste fører deg til page=1 med søkeresultat 2 til 10

@gunnarvelle
Copy link
Member

Nytt søk gir deg ingen mulighet til å få treff utenfor taksonomi, siden du enten sender inn topic-article eller liste med resourceTypes. For å få til det må du nesten sende inn contextType=standard og så berre sende resourceTypes dersom nokon faktisk velger en slik.

Jonas-C
Jonas-C commented Feb 13, 2025
View reviewed changes
src/containers/SearchPageV2/GrepFilter.tsx Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Koden som er kommentert ut i denne filen har å gjøre med visning av navn og beskrivelse av KM/KE, og skal kanskje inn igjen.

@katrinewi
Copy link
Contributor

Burde sidetall resettes når man endrer filter/søket?

@katrinewi
Copy link
Contributor

Virker ikke som treff som vises ved siden av checkboksene samsvarer med faktisk antall søketreff?

@katrinewi
Copy link
Contributor

Mobilvisning burde fikses 😅 pagination overflower :(

@gunnarvelle gunnarvelle requested a review from ghveem February 13, 2025 09:10
@gunnarvelle
Copy link
Member

Ser du over oversettelsestekster @ghveem ?

ghveem
ghveem reviewed Feb 13, 2025
View reviewed changes
ghveem
ghveem requested changes Feb 13, 2025
View reviewed changes
Copy link
Collaborator

@ghveem ghveem left a comment

Choose a reason for hiding this comment

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

Håpar det er forståeleg.
nb: linje 68, 70, 79, 100

katrinewi
katrinewi reviewed Feb 13, 2025
View reviewed changes
@Jonas-C Jonas-C force-pushed the feat/new-search branch 4 times, most recently from 8988553 to 54306a2 Compare February 13, 2025 13:24
@Jonas-C Jonas-C force-pushed the feat/new-search branch 8 times, most recently from 7b54fee to e9bddeb Compare March 11, 2025 16:29
Jonas-C
Jonas-C commented Mar 12, 2025
View reviewed changes
src/containers/SearchPage/GrepFilter.tsx
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Kommentert ut kode er "gammel" visning av grep-koder. Dersom vi går for den forenklede versjonen fjerner jeg det.

@Jonas-C Jonas-C marked this pull request as ready for review March 12, 2025 09:04
@Jonas-C Jonas-C requested a review from a team March 12, 2025 09:04
@gunnarvelle
Copy link
Member

Lenke til søk fra kompetansemål velger ikkje faget. https://ndla-frontend-pr-2296.vercel.app/search?subjects=urn:subject:1:792414c5-896f-470a-9558-6101d7266237&grepCodes=KM2603
Om eg fjerner urn:subject: funker det. Lurer på om det er lurt å ha heile tax-id i søket.

@gunnarvelle
Copy link
Member

Søk etter emne bør også kunne filtreres på fag.

@gunnarvelle
Copy link
Member

Får ingen treff her. Kvifor det? https://ndla-frontend-pr-2296.vercel.app/search?subjects=27e8623d-c092-4f00-9a6f-066438d6c466

@katrinewi
Copy link
Contributor

Forrige/neste knapper i paginering burde være disablet/skjult i mobilvisning også 😅

@katrinewi
Copy link
Contributor

Det virker som pagination viser for mange knapper når man ikke har gjort noen filtrering, finnes hvertfall ingen resultater på side 1750 😅

katrinewi
katrinewi reviewed Mar 12, 2025
View reviewed changes
src/containers/SearchPage/GrepFilter.tsx Outdated
Comment on lines 115 to 117
(!grepQuery.loading && !grepQuery.data?.competenceGoals?.length && !grepQuery.data?.coreElements?.length) ||
(nodeType && nodeType !== RESOURCE_NODE_TYPE)
) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Kan ikke siste sjekken (nodeType && nodeType !== RESOURCE_NODE_TYPE) fjernes ettersom du sjekker for det i skip for grepQuery over?

src/containers/SearchPage/ResourceTypeFilter.tsx

const NODE_TYPES = [SUBJECT_NODE_TYPE, TOPIC_NODE_TYPE, RESOURCE_NODE_TYPE];

export const ResourceTypeFilter = ({ bucketResult, resourceTypes: resourceTypesProp, resourceTypesLoading }: Props) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Jeg syntes denne komponenten er i overkant stor, hadde det gitt mening å dra ut noe av filter-logikken i egne komponenter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Det er bare 50 linjer med logikk filter-logikk, usikker på hvordan noe av det kan flyttes ut.

src/containers/SearchPage/SearchResult.tsx
<DialogCloseButton />
</DialogHeader>
<DialogBody>
<ul>
Copy link
Contributor

Choose a reason for hiding this comment

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

Kunne denne listen kanskje vært UnorderedList?

katrinewi
katrinewi reviewed Mar 13, 2025
View reviewed changes
src/containers/SearchPage/SubjectFilter.tsx
selectedSubjects: string[];
}

const SubjectFilterDialogContent = ({
Copy link
Contributor

Choose a reason for hiding this comment

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

denne til egen fil?

src/containers/SearchPage/SubjectFilter.tsx
}: SubjectFilterDialogContentProps) => {
const { t } = useTranslation();

const tabs = useMemo(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

flytte ut?

src/containers/SearchPage/SubjectFilter.tsx Outdated
title: t(`subjectCategories.${category.type}`),
id: category.type,
message: category.message,
subjects: groupBy(sortedSubjects, (s) => s.name[0]?.toUpperCase() ?? "undefined"),
Copy link
Contributor

Choose a reason for hiding this comment

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

Undefined som en streng?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Det er det lodash gjør dersom man returnerer undefined. Kan endre det til å filtrere ut fag uten navn.

src/util/useStableSearchParams.ts
import { useSearchParams } from "react-router-dom";

// The purpose of this hook is to provide a stable search params object that is always sorted by key. `useSearchParams` from "react-router-dom" does not guarantee the order of the search params.
export const useStableSearchParams = () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Trenger hooksene å hete det samme? Jeg syntes det var litt rart

@Jonas-C Jonas-C requested a review from katrinewi March 13, 2025 08:49
@gunnarvelle
Copy link
Member

Totalcount er 17194
image

@Jonas-C
Copy link
Contributor Author

Jonas-C commented Mar 13, 2025

Totalcount er 17194

Ja, men vi klarer ikke vise frem mer enn 10k. Tenkte det var bedre at pageCount samsvarte med antall sider

@gunnarvelle
Copy link
Member

Totalcount er 17194

Ja, men vi klarer ikke vise frem mer enn 10k. Tenkte det var bedre at pageCount samsvarte med antall sider

Uenig. Totalt antall må stemme. Nokke anna ser berre rart ut. Tipper ingen vil stusse over at du ikkje får paginert etter side 1000. Kan eventuelt droppe muighet til å hoppe til siste side.

@gunnarvelle
Copy link
Member

gunnarvelle commented Mar 13, 2025 •
edited
Loading

Filtrering på kjerneelement funker ikkje. Ikkje kompetansemål heller forsåvidt.

@gunnarvelle
Copy link
Member

Paginering virker ikkje for emner eller fag

@gunnarvelle
Copy link
Member

Vi sender ingen sort parametre. Er det bevisst? Ser at search-api defaulter til relevance men uten query gir ikkje det så masse meining.

@Jonas-C
Copy link
Contributor Author

Jonas-C commented Mar 17, 2025

Vi sender ingen sort parametre. Er det bevisst? Ser at search-api defaulter til relevance men uten query gir ikkje det så masse meining.

Tja, vi kan sortere basert på relevance dersom query er satt?

@gunnarvelle
Copy link
Member

Vi sender ingen sort parametre. Er det bevisst? Ser at search-api defaulter til relevance men uten query gir ikkje det så masse meining.

Tja, vi kan sortere basert på relevance dersom query er satt?

Det gjøres implisitt av backend. Det er søk uten query som kanskje må ha en sort?

@gunnarvelle
Copy link
Member

gunnarvelle commented Mar 17, 2025 •
edited
Loading

Skulle gjerne hatt ut NDLANO/backend#624 for å begrense fagsøket. ✔️

@Jonas-C Jonas-C requested review from gunnarvelle and a team March 18, 2025 09:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ghveem ghveem ghveem requested changes

@katrinewi katrinewi Awaiting requested review from katrinewi

@gunnarvelle gunnarvelle Awaiting requested review from gunnarvelle

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Jonas-C @gunnarvelle @katrinewi @ghveem