Feature/arc 3819 copyright - #367
Conversation
| imageAction, | ||
| imageAlt, | ||
| title = '', | ||
| copyrightIconVisible = false, |
There was a problem hiding this comment.
De FA zegt "Copyright icoon toevoegen: checkbox (True or false). Default = true." en dat doe je ook netjes in de state (defaults.ts:239 en BlockImage.editorconfig.ts:23 zetten allebei true).
Maar de component-defaults staan op false: hier, in CopyrightAttribution.tsx:17 en in BlockRichText.tsx:43. Voor bestaande opgeslagen blokken zit copyrightIconVisible niet in de state, dus die vallen terug op false en verliezen hun ©-icoon.
Ik kan me voorstellen dat dat net de bedoeling is — het ticket wil immers dat het symbool "niet automatisch getoond wordt" — maar dan wijkt het af van de "Default = true" in de FA, en spreken de state-defaults en de component-defaults elkaar tegen. Wat is hier de bedoeling voor bestaande content?
| ...COPYRIGHT_FIELDS({ | ||
| title: { | ||
| fieldName: 'title', | ||
| }, | ||
| text: { | ||
| fieldName: 'text', | ||
| }, | ||
| }), |
There was a problem hiding this comment.
Deze twee velden hadden hiervoor bestaande, correcte sleutels:
admin/content-block/helpers/generators/image___bijschift-titeladmin/content-block/helpers/generators/image___bijschrift-beschrijving
Nu vallen ze terug op de generieke labels uit COPYRIGHT_FIELDS ('bijschrift titel' / 'bijschrift beschrijving'), dus die twee vertalingen gaan verloren in contentbeheer.
Je overrides-mechanisme kan dat wel opvangen — title: { fieldName: 'title', overrides: { label: tText('…image___bijschift-titel') } }. Bewust laten vallen of over het hoofd gezien?
| }; | ||
| }): ContentBlockComponentsConfig['fields'] => ({ | ||
| [overrides?.title?.fieldName || 'copyrightTitle']: TEXT_FIELD({ | ||
| label: tText('bijschrift titel'), |
There was a problem hiding this comment.
tText('bijschrift titel'), tText('Toon bijschrift icoon') (r226) en tText('bijschrift beschrijving') (r231) gebruiken de ruwe string als key. Omdat dit nu in 6 blokken gedeeld wordt is dat extra jammer — de rest van defaults.ts gebruikt overal admin/content-block/helpers/generators/defaults___….
Zou dat niet iets als admin/content-block/helpers/generators/defaults___bijschrift-titel moeten zijn? (De casing verschilt nu onderling ook: twee lowercase, één met hoofdletter.)
| .c-block-het-archief-image-text-background__image-wrapper { | ||
| @media (min-width: variables.$g-bp2) { | ||
| position: absolute; | ||
| inset: 0; |
There was a problem hiding this comment.
inset: 0 doet niets meer zonder position: absolute, dus deze hele media query kan mee weg.
Belangrijker: dit is een reële layout-wijziging op ≥$g-bp2 voor een bestaand blok — de image-wrapper ging van absoluut gepositioneerd (vullend binnen zijn grid-area, met img { height: 100% } daarop gebaseerd) naar een gewone grid-item die nu óók het bijschrift bevat. Heb je de vier alignment-varianten (--left-screen, --right-screen, --left-inside-page, --right-inside-page) op desktop en mobiel nagekeken?
En eentje die daarmee samenhangt: regel 126 (&.c-image img onder --right-inside-page) matcht niet meer. avo2's Image rendert <div class="{className} c-image">, dus vóór deze PR zaten __image-wrapper en c-image op hetzelfde element. Nu staat __image-wrapper op de nieuwe <div> en c-image op de <Image> daarbinnen, waardoor die margin-left: auto !important wegvalt.
| imageAlignment === 'right-screen', | ||
| })} | ||
| alt={imageAltText} | ||
| <div className={'c-block-het-archief-image-text-background__image-wrapper'}> |
There was a problem hiding this comment.
Hiervoor stond er {image && <Image … />}, nu wordt deze <div> altijd gerenderd — ook als er geen afbeelding is en CopyrightAttribution null teruggeeft. Dan blijft er een lege grid-cel over die wel ruimte inneemt in de grid.
Zou dit niet beter {(image || copyrightTitle || copyrightText) && (…)} zijn, of de wrapper enkel bij een afbeelding?
| copyrightTitle: string; | ||
| copyrightIconVisible: boolean; | ||
| copyrightText: string; |
There was a problem hiding this comment.
Deze drie staan hier als verplicht en worden zonder default gedestructureerd, terwijl bestaande opgeslagen blokken die keys niet hebben — dus in de praktijk zijn ze undefined. Runtime valt dat mee omdat CopyrightAttribution zelf defaults heeft, maar het type klopt niet.
In BlockImage.tsx:18 en BlockRichText.tsx:23-26 heb je ze wél optioneel gemaakt, dus ik zou het hier gelijktrekken.
Zelfde bedenking bij de types: ImageGridBlockComponentStateFields extends CopyrightComponentState maakt ze verplicht (content-block.types.ts:286), terwijl GridItem in BlockImageGrid.types.tsx ze optioneel heeft. Bewust twee verschillende varianten?
| import './CopyrightAttribution.scss'; | ||
| import clsx from 'clsx'; | ||
|
|
||
| export interface BlockImageProps extends DefaultProps { |
There was a problem hiding this comment.
Deze interface heet BlockImageProps (copy-paste denk ik?). Ze wordt via export * from './CopyrightAttribution.tsx' in index.ts mee geëxporteerd, dus er zijn nu twee BlockImageProps in de codebase — deze en die in BlockImage.tsx. CopyrightAttributionProps?
| if (!title && !text) { | ||
| return null; | ||
| } | ||
|
|
||
| const renderTitle = () => { | ||
| if (!title && !showIcon) { | ||
| return null; | ||
| } | ||
|
|
||
| return ( | ||
| <span className="a-copyright-attribution__annotation"> | ||
| {showIcon && <>©</>} {title} |
There was a problem hiding this comment.
Als title leeg is maar text gevuld en showIcon op true staat, passeert de guard op regel 20 en rendert renderTitle() een <span>© </span> zonder enige bronvermelding erachter — een los copyright-symbool.
Gezien het ticket net gaat over "geen verwarring scheppen rond de rechtenstatus" lijkt me dat ongewenst. Zou het icoon niet enkel getoond mogen worden als er ook een title is?
| <CopyrightAttribution | ||
| title={element.copyrightTitle} | ||
| text={element.copyrightText} | ||
| showIcon={element.copyrightIconVisible} | ||
| /> |
There was a problem hiding this comment.
renderGridImage(element) wordt verderop volledig in renderLink(element.action, …) gewikkeld, dus de bronvermelding wordt onderdeel van het klikbare gebied én van de accessible name van de link.
Voor de titel/tekst was dat al zo, dus het is consistent met wat er stond — maar voor een copyright-bijschrift voelt dat anders. Bewust binnen de link gezet, of zou die er beter buiten staan?
2 participants
No description provided.