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

[FINNA-1454] AIPA: Record page style, display and text changes #2806

Merged
merged 13 commits into from
Dec 13, 2023

Conversation

aleksip
Copy link

@aleksip aleksip commented Dec 8, 2023

No description provided.

@aleksip aleksip requested a review from pasitiis December 8, 2023 10:00
@aleksip aleksip requested a review from EreMaijala December 8, 2023 10:40
@aleksip
Copy link
Author

aleksip commented Dec 8, 2023

@EreMaijala Tätä saa mielellään jo katselmoida, mutta jos pidetään mergeämättä viimeiseen asti, niin ei tarvi avata kolmatta viimeistely-PR:ää.

Comment on lines 5 to 6
unset($defaults['Language']); // Not displayed
unset($defaults['Subjects']); // Subjects are displayed in a different part of the record page

Choose a reason for hiding this comment

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

Olisi varmaan ainakin yhtenäisempää laittaa oma filtteri getDefaults-metodiin.

@aleksip aleksip marked this pull request as ready for review December 12, 2023 13:30
@aleksip aleksip requested a review from EreMaijala December 12, 2023 13:30
Copy link

@EreMaijala EreMaijala left a comment

Choose a reason for hiding this comment

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

Näyttää muuten ihan hyvältä, mutta jäin pohtimaan, sitä kun noi CuratedRecord:n result-templaatit puskevat useita muuttujia globaaliin kontekstiin eivätkä siivoa jälkiä, niin onko tilannetta, jossa esim. listNoteTitle jää sinne myös muille tietueille? Olisi selkeämpää, jos noi templaatit kutsuisivat suoraan Record-helperin getSearchResult:n sijaan renderTemplate-metodia, jolle voi antaa parametrina tarvittavat muuttujat, jotka se lisää globaaliin kontekstiin väliaikaisesti. Tämä varsinkin, kun tällä hetkellä getSearchResult:lle annetaan kaksi parametria, vaikka se ottaa vain yhden, eikö?

@aleksip aleksip requested a review from EreMaijala December 13, 2023 08:45
@EreMaijala EreMaijala merged commit 5c8435c into NatLibFi:dev Dec 13, 2023
6 checks passed
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