feat/thumbnail #3898
feat/thumbnail #3898
Conversation
…nto Feat/Thumbnail
walldenfilippa
added
kind/product-feature
📝 WalkthroughWalkthroughThis PR introduces image thumbnail support to file upload components. It adds translation keys across English, Norwegian Bokmål, and Norwegian Nynorsk, creates thumbnail display and preview components with responsive CSS styling, updates file table components to conditionally render thumbnails based on attachment metadata, and extends the IData type with metadata fields. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
old PR: #3797 |
|
/publish |
PR release:
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| }) => { | ||
| const { langAsString } = useLanguage(); | ||
| const uniqueId = isAttachmentUploaded(attachment) ? attachment.data.id : attachment.data.temporaryId; | ||
| return ( |
Check warning
Code scanning / CodeQL
Useless conditional Warning
|
/publish |
PR release:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Fix all issues with AI Agents 🤖
In @src/layout/FileUpload/FileUploadTable/AttachmentThumbnail.tsx:
- Around line 42-58: AttachmentThumbnail renders an <img> with src={url} where
url can be undefined and wraps it in a div faking button behavior; fix by (1)
guarding the image src: if url is falsy, render a placeholder image or omit the
<img> entirely and/or add an alt that indicates no preview available (update the
usage of the url variable and the <img> element in AttachmentThumbnail), and (2)
replace the interactive div (classes.thumbnailContainer with
role/tabIndex/onKeyDown) with a native <button> when onThumbnailClick is
provided (or render a non-interactive container when not), wiring onClick to
onThumbnailClick and preserving classes.thumbnailContainer and
mobileView-dependent class names while removing custom keyboard handlers so
native button accessibility is used.
In @src/layout/FileUpload/FileUploadTable/FileTable.tsx:
- Line 52: thumbnailTitle is set to undefined when mobileView is true but the
thumbnail <th> is still rendered when hasImages && !pdfModeActive, causing Lang
to receive an undefined id; update the thumbnail header to mirror the other
desktop-only columns by only rendering the <th> (or setting thumbnailTitle) when
!mobileView && hasImages && !pdfModeActive so Lang always gets a valid id
(reference thumbnails: the thumbnailTitle const, mobileView, hasImages,
pdfModeActive, and the Lang component used inside the <th>).
In @src/layout/FileUpload/FileUploadTable/FileTableRow.tsx:
- Around line 174-202: Inside the mobileView conditional block in FileTableRow
(the JSX starting with "{mobileView && ("), remove redundant and unreachable
checks: drop the extra "mobileView" guard from the tag label render (change
"{tagLabel && mobileView && (" to simply "{tagLabel && ("), replace the
always-true ternary "${mobileView ? uploadStatus : ''}" with just
"${uploadStatus}", and delete the unreachable block guarded by "{hasTag &&
!mobileView && (" since "!mobileView" can never be true here; keep the
AltinnLoader fallback and ensure Lang, tagLabel, uploadStatus, hasTag, uniqueId
and classes.altinnLoader usages remain otherwise unchanged.
In @src/layout/FileUpload/FileUploadTable/ThumbnailPreviewModal.tsx:
- Around line 48-70: When opening ThumbnailPreviewModal, remove role='button'
and tabIndex={0} from classes.previewBackdrop and instead set role='dialog' and
aria-modal='true' on the modal container (the div with classes.previewModal) and
add aria-labelledby that references an id you add to the fileName span;
implement focus management in the component: save document.activeElement before
open, move focus into a focusable element inside the modal (e.g., the close
button) in a useEffect when the modal mounts, trap focus within the modal by
handling Tab/Shift+Tab in the modal's onKeyDown or using a small focus-trap
utility, and onClose restore focus to the previously focused trigger; keep
backdrop click behavior via handleBackdropClick and keep Escape handling but
ensure it comes from the modal container's keydown handler so focus remains
controlled.
- Around line 79-85: ThumbnailPreviewModal currently only uses handleImageLoad
on the <img> so a failed fetch leaves the spinner visible; add an onError
handler (e.g. handleImageError) that sets the same loading state to false and
records an error flag (e.g. setImageLoadError(true)) so the spinner is removed
and the UI can show a fallback (placeholder icon/alt text) or an error message;
update the render logic that currently reads isImageLoading to also check the
error flag and show the fallback UI (or a retry action) instead of the image
when imageLoadError is true.
🧹 Nitpick comments (6)
src/types/shared.ts (1)
24-27: Consider whether fully optional fields are appropriate.Both
keyandvalueare optional, which allows{}to be a validIMetadataobject. While this provides flexibility, it may allow meaningless metadata entries.If both fields should always be present when metadata is provided, consider:
export interface IMetadata { - key?: string; - value?: string; + key: string; + value: string; }However, if the flexibility is intentional for future extensibility, the current design is acceptable.
src/layout/FileUpload/FileUploadTable/ThumbnailPreviewModal.tsx (3)
28-32: Loading state doesn't reset when modal reopens.The
isImageLoadingstate is initialized totrueonly once. If the modal closes and reopens with the same component instance (whenisOpentoggles), the loading state won't reset, potentially showing the previous image immediately without a loading indicator.🔎 Suggested fix to reset loading state
+React.useEffect(() => { + if (isOpen) { + setIsImageLoading(true); + } +}, [isOpen, attachment]);
63-66: Use translated text for close button aria-label.The
aria-label='Close preview'is hardcoded in English. This should use a translation key to support the three languages (en, nb, nn) already present in the codebase.
71-78: Use translated text for loading spinner aria-label.The spinner's
aria-label='Loading'is hardcoded in English and should use a translation key for consistency with the rest of the application.src/layout/FileUpload/FileUploadTable/AttachmentThumbnail.module.css (1)
34-93: Replace hard-coded colors with design system tokens.The modal styles use hard-coded color values (
#fff,#333) instead of CSS custom properties from the design system. For consistency and maintainability, use design tokens like thecloseButtondoes (lines 101, 114-115).🔎 Proposed refactor to use design tokens
.previewModal { position: relative; - background-color: #fff; + background-color: var(--ds-color-surface-default); border-radius: 8px; box-shadow: 0 4px 20px rgba(0, 0, 0, 0.3); max-width: 90vw; max-height: 90vh; overflow: auto; padding: 0; display: flex; flex-direction: column; } .previewHeader { position: relative; display: flex; justify-content: space-between; align-items: center; padding: 16px 20px; - background-color: #fff; + background-color: var(--ds-color-surface-default); border-radius: 8px 8px 0 0; gap: 16px; } .previewHeaderMobile { position: relative; display: flex; justify-content: space-between; align-items: center; padding: 8px 20px; - background-color: #fff; + background-color: var(--ds-color-surface-default); border-radius: 8px 8px 0 0; gap: 8px; } .fileName { font-weight: 500; - color: #333; + color: var(--ds-color-text-default); flex: 1; word-break: break-word; font-size: 0.95rem; } .previewImage { max-width: 100%; max-height: calc(90vh - 100px); object-fit: contain; display: block; padding: 0 20px 20px 20px; - background-color: #fff; + background-color: var(--ds-color-surface-default); } .previewImageMobile { max-width: 100%; max-height: calc(90vh - 100px); object-fit: contain; display: block; padding: 0 10px 10px 10px; - background-color: #fff; + background-color: var(--ds-color-surface-default); }As per coding guidelines, leverage Digdir Design System components and tokens when possible.
src/layout/FileUpload/FileUploadTable/AttachmentThumbnail.tsx (1)
23-25: Returnnullinstead of empty string.React components should return
nullfor conditional rendering, not empty strings. While empty strings work,nullis the idiomatic React convention and more semantic.🔎 Proposed fix
// Only uploaded attachments can have thumbnails if (!instanceId || !isAttachmentUploaded(attachment)) { - return ''; + return null; } const thumbnailLink = attachment.data.metadata?.find((meta) => meta.key === 'thumbnailLink')?.value; if (!thumbnailLink) { - return ''; + return null; }Also applies to: 28-30
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/language/texts/en.tssrc/language/texts/nb.tssrc/language/texts/nn.tssrc/layout/FileUpload/FileUploadTable/AttachmentThumbnail.module.csssrc/layout/FileUpload/FileUploadTable/AttachmentThumbnail.tsxsrc/layout/FileUpload/FileUploadTable/FileTable.tsxsrc/layout/FileUpload/FileUploadTable/FileTableRow.tsxsrc/layout/FileUpload/FileUploadTable/ThumbnailPreviewModal.tsxsrc/types/shared.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Avoid usinganytype or type casting (as type) in TypeScript code; improve typing by avoiding casts andanys when refactoring
Use objects for managing query keys and functions, andqueryOptionsfor sharing TanStack Query patterns across the system for central management
Files:
src/types/shared.tssrc/language/texts/nn.tssrc/language/texts/en.tssrc/layout/FileUpload/FileUploadTable/AttachmentThumbnail.tsxsrc/language/texts/nb.tssrc/layout/FileUpload/FileUploadTable/ThumbnailPreviewModal.tsxsrc/layout/FileUpload/FileUploadTable/FileTableRow.tsxsrc/layout/FileUpload/FileUploadTable/FileTable.tsx
{**/*.module.css,**/*.{ts,tsx}}
📄 CodeRabbit inference engine (CLAUDE.md)
Use CSS Modules for component styling and leverage Digdir Design System components when possible
Files:
src/types/shared.tssrc/language/texts/nn.tssrc/language/texts/en.tssrc/layout/FileUpload/FileUploadTable/AttachmentThumbnail.tsxsrc/language/texts/nb.tssrc/layout/FileUpload/FileUploadTable/AttachmentThumbnail.module.csssrc/layout/FileUpload/FileUploadTable/ThumbnailPreviewModal.tsxsrc/layout/FileUpload/FileUploadTable/FileTableRow.tsxsrc/layout/FileUpload/FileUploadTable/FileTable.tsx
🧠 Learnings (1)
📚 Learning: 2025-10-14T09:01:40.985Z
Learnt from: lassopicasso
Repo: Altinn/app-frontend-react PR: 3654
File: src/layout/ImageUpload/ImageUploadSummary2/ImageUploadSummary2.tsx:49-53
Timestamp: 2025-10-14T09:01:40.985Z
Learning: In the Altinn app-frontend-react codebase, when data integrity is guaranteed through business logic and parent components guard rendering (e.g., `storedImage ? <Component /> : undefined`), non-null assertions on guaranteed fields like `storedImage!.data?.filename` are acceptable and preferred over optional chaining with fallbacks.
Applied to files:
src/layout/FileUpload/FileUploadTable/FileTableRow.tsx
🧬 Code graph analysis (2)
src/layout/FileUpload/FileUploadTable/AttachmentThumbnail.tsx (5)
src/features/attachments/index.ts (2)
IAttachment(22-22)isAttachmentUploaded(29-31)src/features/instance/InstanceContext.tsx (2)
useInstanceDataElements(139-143)useLaxInstanceId(58-62)src/features/language/LanguageProvider.tsx (1)
useCurrentLanguage(80-80)src/utils/urls/urlHelper.ts (1)
makeUrlRelativeIfSameDomain(129-139)src/utils/urls/appUrlHelper.ts (1)
getDataElementUrl(78-79)
src/layout/FileUpload/FileUploadTable/FileTable.tsx (8)
src/features/attachments/index.ts (2)
IAttachment(22-22)isAttachmentUploaded(29-31)src/features/options/castOptionsToStrings.ts (1)
IOptionInternal(3-14)src/utils/layout/useNodeItem.ts (1)
useItemWhenType(15-33)src/features/pdf/PdfWrapper.tsx (1)
usePdfModeActive(14-18)src/utils/formComponentUtils.ts (1)
atLeastOneTagExists(20-29)src/features/language/Lang.tsx (1)
Lang(15-23)src/layout/FileUpload/FileUploadTable/FileTableRowContext.tsx (2)
FileTableRowContext(3-7)FileTableRowProvider(11-11)src/layout/FileUpload/FileUploadTable/FileTableRow.tsx (1)
FileTableRow(33-148)
🪛 GitHub Check: CodeQL
src/layout/FileUpload/FileUploadTable/FileTableRow.tsx
[warning] 166-166: Useless conditional
This use of variable 'mobileView' always evaluates to true.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Install
🔇 Additional comments (8)
src/language/texts/en.ts (1)
77-77: LGTM! Translation addition is clear and well-positioned.The new translation key follows the existing naming convention and is appropriately placed among related file uploader header translations.
src/types/shared.ts (1)
59-59: LGTM! Metadata field properly added with backward compatibility.The optional metadata field is well-typed and maintains backward compatibility. The inline comment is helpful, though the field could serve purposes beyond thumbnails in the future.
src/language/texts/nn.ts (1)
78-78: LGTM! Norwegian Nynorsk translation added consistently.The translation key and placement are consistent with the corresponding additions in other language files.
src/language/texts/nb.ts (1)
78-78: LGTM! Norwegian Bokmål translation added consistently.The translation follows the same pattern as the other language files and is properly positioned.
src/layout/FileUpload/FileUploadTable/ThumbnailPreviewModal.tsx (1)
11-17: LGTM! Interface is well-defined with clear prop types.The props are appropriately typed and the optional
mobileViewprovides flexibility for responsive behavior.src/layout/FileUpload/FileUploadTable/FileTableRow.tsx (2)
113-121: LGTM! Thumbnail cell rendering is correctly guarded.The thumbnail cell is only rendered when both
hasImagesand!pdfModeActiveare true, which properly aligns with the header rendering logic in FileTable.tsx. The click handler integration withAttachmentThumbnailis clean.
139-145: LGTM! Preview modal integration looks solid.The
ThumbnailPreviewModalis properly controlled by local state and includes appropriate fallback handling for the filename. The modal is rendered outside the table row to avoid layout issues.src/layout/FileUpload/FileUploadTable/AttachmentThumbnail.module.css (1)
30-30: The z-index value is consistent with the application's modal overlay standard.The
z-index: 1000on.previewBackdropaligns with other modal and overlay components in the codebase (DevTools, OpenDevToolsButton, etc.), suggesting this is the established pattern for such elements. No change needed unless the application's z-index scale is being formally revised.
| return ( | ||
| <div | ||
| className={classes.thumbnailContainer} | ||
| data-testid='attachment-thumbnail' | ||
| onClick={onThumbnailClick} | ||
| style={onThumbnailClick ? { cursor: 'pointer' } : {}} | ||
| role={onThumbnailClick ? 'button' : undefined} | ||
| tabIndex={onThumbnailClick ? 0 : undefined} | ||
| onKeyDown={onThumbnailClick ? (e) => e.key === 'Enter' && onThumbnailClick() : undefined} | ||
| > | ||
| <img | ||
| src={url} | ||
| alt={`Thumbnail for ${attachment.data.filename}`} | ||
| className={mobileView ? classes.thumbnailMobile : classes.thumbnail} | ||
| /> | ||
| </div> | ||
| ); |
There was a problem hiding this comment.
Improve accessibility and add safety check for image URL.
Two concerns:
-
Missing URL validation: The
urlcan beundefinedif nothumbnailDataElementis found, yet it's passed directly to theimgsrcattribute without validation. This could cause a broken image display. -
Accessibility pattern: Using a
divwith button-like behavior (role, tabIndex, onClick, onKeyDown) is non-standard. While functional, a native<button>element provides better accessibility and keyboard navigation out of the box.
🔎 Proposed fixes
Add URL validation:
const url = thumbnailDataElement
? makeUrlRelativeIfSameDomain(getDataElementUrl(instanceId, thumbnailDataElement.id, language))
: undefined;
+ if (!url) {
+ return null;
+ }
+
return (Alternative: Use a button wrapper for better accessibility (optional, if styling allows):
return (
- <div
+ <button
+ type="button"
className={classes.thumbnailContainer}
data-testid='attachment-thumbnail'
onClick={onThumbnailClick}
- style={onThumbnailClick ? { cursor: 'pointer' } : {}}
- role={onThumbnailClick ? 'button' : undefined}
- tabIndex={onThumbnailClick ? 0 : undefined}
- onKeyDown={onThumbnailClick ? (e) => e.key === 'Enter' && onThumbnailClick() : undefined}
+ disabled={!onThumbnailClick}
>
<img
src={url}
alt={`Thumbnail for ${attachment.data.filename}`}
className={mobileView ? classes.thumbnailMobile : classes.thumbnail}
/>
- </div>
+ </button>
);Note: If using a button, you may need to add CSS to remove default button styling.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return ( | |
| <div | |
| className={classes.thumbnailContainer} | |
| data-testid='attachment-thumbnail' | |
| onClick={onThumbnailClick} | |
| style={onThumbnailClick ? { cursor: 'pointer' } : {}} | |
| role={onThumbnailClick ? 'button' : undefined} | |
| tabIndex={onThumbnailClick ? 0 : undefined} | |
| onKeyDown={onThumbnailClick ? (e) => e.key === 'Enter' && onThumbnailClick() : undefined} | |
| > | |
| <img | |
| src={url} | |
| alt={`Thumbnail for ${attachment.data.filename}`} | |
| className={mobileView ? classes.thumbnailMobile : classes.thumbnail} | |
| /> | |
| </div> | |
| ); | |
| return ( | |
| <button | |
| type="button" | |
| className={classes.thumbnailContainer} | |
| data-testid='attachment-thumbnail' | |
| onClick={onThumbnailClick} | |
| disabled={!onThumbnailClick} | |
| > | |
| <img | |
| src={url} | |
| alt={`Thumbnail for ${attachment.data.filename}`} | |
| className={mobileView ? classes.thumbnailMobile : classes.thumbnail} | |
| /> | |
| </button> | |
| ); |
🤖 Prompt for AI Agents
In @src/layout/FileUpload/FileUploadTable/AttachmentThumbnail.tsx around lines
42-58, AttachmentThumbnail renders an <img> with src={url} where url can be
undefined and wraps it in a div faking button behavior; fix by (1) guarding the
image src: if url is falsy, render a placeholder image or omit the <img>
entirely and/or add an alt that indicates no preview available (update the usage
of the url variable and the <img> element in AttachmentThumbnail), and (2)
replace the interactive div (classes.thumbnailContainer with
role/tabIndex/onKeyDown) with a native <button> when onThumbnailClick is
provided (or render a non-interactive container when not), wiring onClick to
onThumbnailClick and preserving classes.thumbnailContainer and
mobileView-dependent class names while removing custom keyboard handlers so
native button accessibility is used.
| const firstTag = attachment.data.tags && attachment.data.tags[0]; | ||
| return options?.find((option) => option.value === firstTag)?.label; | ||
| }; | ||
| const thumbnailTitle = !mobileView ? 'form_filler.file_uploader_list_header_thumbnail' : undefined; |
There was a problem hiding this comment.
Fix undefined translation key in mobile view.
The thumbnailTitle is set to undefined when mobileView is true (line 52), but the thumbnail header <th> (lines 99-103) is rendered when hasImages && !pdfModeActive without checking mobileView. This causes Lang to receive undefined as the id prop in mobile view, which will break the translation.
Looking at other headers (lines 84, 89, 94), they all include !mobileView checks for desktop-only columns. The thumbnail header should follow the same pattern.
🔎 Proposed fix
- const thumbnailTitle = !mobileView ? 'form_filler.file_uploader_list_header_thumbnail' : undefined;
+ const thumbnailTitle = 'form_filler.file_uploader_list_header_thumbnail';- {hasImages && !pdfModeActive && (
+ {hasImages && !pdfModeActive && !mobileView && (
<th>
<Lang id={thumbnailTitle} />
</th>
)}Also applies to: 99-103
🤖 Prompt for AI Agents
In @src/layout/FileUpload/FileUploadTable/FileTable.tsx around line 52,
thumbnailTitle is set to undefined when mobileView is true but the thumbnail
<th> is still rendered when hasImages && !pdfModeActive, causing Lang to receive
an undefined id; update the thumbnail header to mirror the other desktop-only
columns by only rendering the <th> (or setting thumbnailTitle) when !mobileView
&& hasImages && !pdfModeActive so Lang always gets a valid id (reference
thumbnails: the thumbnailTitle const, mobileView, hasImages, pdfModeActive, and
the Lang component used inside the <th>).
| {mobileView && ( | ||
| <div | ||
| style={{ | ||
| color: AltinnPalette.grey, | ||
| }} | ||
| > | ||
| {attachment.uploaded ? ( | ||
| <div> | ||
| {tagLabel && mobileView && ( | ||
| <div> | ||
| <Lang id={tagLabel} /> | ||
| </div> | ||
| )} | ||
| {`${readableSize} ${mobileView ? uploadStatus : ''}`} | ||
| {hasTag && !mobileView && ( | ||
| <div data-testid='status-success'> | ||
| <Lang id='form_filler.file_uploader_list_status_done' /> | ||
| </div> | ||
| )} | ||
| </div> | ||
| ) : ( | ||
| <AltinnLoader | ||
| id={`attachment-loader-upload-${uniqueId}`} | ||
| className={classes.altinnLoader} | ||
| srContent={langAsString('general.loading')} | ||
| /> | ||
| )} | ||
| </div> | ||
| )} |
There was a problem hiding this comment.
Remove redundant conditions and unreachable code.
Within the {mobileView && ( block starting at line 174, there are redundant and contradictory checks on mobileView:
- Line 182:
{tagLabel && mobileView && (— ThemobileViewcheck is redundant since we're already inside amobileViewblock. - Line 187:
${mobileView ? uploadStatus : ''}— The ternary always evaluates to the true branch. - Lines 188-192:
{hasTag && !mobileView && (— This condition is always false, making this entire code block unreachable dead code.
This aligns with the CodeQL warning flagging useless conditionals.
🔎 Proposed fix to remove redundant checks
{mobileView && (
<div
style={{
color: AltinnPalette.grey,
}}
>
{attachment.uploaded ? (
<div>
- {tagLabel && mobileView && (
+ {tagLabel && (
<div>
<Lang id={tagLabel} />
</div>
)}
- {`${readableSize} ${mobileView ? uploadStatus : ''}`}
- {hasTag && !mobileView && (
- <div data-testid='status-success'>
- <Lang id='form_filler.file_uploader_list_status_done' />
- </div>
- )}
+ {`${readableSize} ${uploadStatus}`}
</div>
) : (
<AltinnLoader
id={`attachment-loader-upload-${uniqueId}`}
className={classes.altinnLoader}
srContent={langAsString('general.loading')}
/>
)}
</div>
)}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {mobileView && ( | |
| <div | |
| style={{ | |
| color: AltinnPalette.grey, | |
| }} | |
| > | |
| {attachment.uploaded ? ( | |
| <div> | |
| {tagLabel && mobileView && ( | |
| <div> | |
| <Lang id={tagLabel} /> | |
| </div> | |
| )} | |
| {`${readableSize} ${mobileView ? uploadStatus : ''}`} | |
| {hasTag && !mobileView && ( | |
| <div data-testid='status-success'> | |
| <Lang id='form_filler.file_uploader_list_status_done' /> | |
| </div> | |
| )} | |
| </div> | |
| ) : ( | |
| <AltinnLoader | |
| id={`attachment-loader-upload-${uniqueId}`} | |
| className={classes.altinnLoader} | |
| srContent={langAsString('general.loading')} | |
| /> | |
| )} | |
| </div> | |
| )} | |
| {mobileView && ( | |
| <div | |
| style={{ | |
| color: AltinnPalette.grey, | |
| }} | |
| > | |
| {attachment.uploaded ? ( | |
| <div> | |
| {tagLabel && ( | |
| <div> | |
| <Lang id={tagLabel} /> | |
| </div> | |
| )} | |
| {`${readableSize} ${uploadStatus}`} | |
| </div> | |
| ) : ( | |
| <AltinnLoader | |
| id={`attachment-loader-upload-${uniqueId}`} | |
| className={classes.altinnLoader} | |
| srContent={langAsString('general.loading')} | |
| /> | |
| )} | |
| </div> | |
| )} |
🤖 Prompt for AI Agents
In @src/layout/FileUpload/FileUploadTable/FileTableRow.tsx around lines 174-202,
Inside the mobileView conditional block in FileTableRow (the JSX starting with
"{mobileView && ("), remove redundant and unreachable checks: drop the extra
"mobileView" guard from the tag label render (change "{tagLabel && mobileView &&
(" to simply "{tagLabel && ("), replace the always-true ternary "${mobileView ?
uploadStatus : ''}" with just "${uploadStatus}", and delete the unreachable
block guarded by "{hasTag && !mobileView && (" since "!mobileView" can never be
true here; keep the AltinnLoader fallback and ensure Lang, tagLabel,
uploadStatus, hasTag, uniqueId and classes.altinnLoader usages remain otherwise
unchanged.
| return ( | ||
| <div | ||
| className={classes.previewBackdrop} | ||
| onClick={handleBackdropClick} | ||
| onKeyDown={(e) => { | ||
| if (e.key === 'Escape') { | ||
| onClose(); | ||
| } | ||
| }} | ||
| role='button' | ||
| tabIndex={0} | ||
| > | ||
| <div className={classes.previewModal}> | ||
| <div className={mobileView ? classes.previewHeaderMobile : classes.previewHeader}> | ||
| <span className={classes.fileName}>{fileName}</span> | ||
| <button | ||
| className={classes.closeButton} | ||
| onClick={onClose} | ||
| aria-label='Close preview' | ||
| > | ||
| × | ||
| </button> | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add focus management and improve modal accessibility.
The modal lacks proper focus management:
- Focus should move into the modal when it opens
- Focus should be trapped within the modal while open
- Focus should return to the trigger element when closed
Additionally, the backdrop's role='button' and tabIndex={0} are not appropriate—a backdrop shouldn't be focusable as a button. Consider removing these or using role='dialog' and aria-modal='true' on the modal container instead.
The modal should also have aria-labelledby pointing to the fileName element for screen readers.
🤖 Prompt for AI Agents
In @src/layout/FileUpload/FileUploadTable/ThumbnailPreviewModal.tsx around lines
48-70, When opening ThumbnailPreviewModal, remove role='button' and tabIndex={0}
from classes.previewBackdrop and instead set role='dialog' and aria-modal='true'
on the modal container (the div with classes.previewModal) and add
aria-labelledby that references an id you add to the fileName span; implement
focus management in the component: save document.activeElement before open, move
focus into a focusable element inside the modal (e.g., the close button) in a
useEffect when the modal mounts, trap focus within the modal by handling
Tab/Shift+Tab in the modal's onKeyDown or using a small focus-trap utility, and
onClose restore focus to the previously focused trigger; keep backdrop click
behavior via handleBackdropClick and keep Escape handling but ensure it comes
from the modal container's keydown handler so focus remains controlled.
| <img | ||
| src={url} | ||
| alt={fileName} | ||
| className={mobileView ? classes.previewImageMobile : classes.previewImage} | ||
| onLoad={handleImageLoad} | ||
| style={{ display: isImageLoading ? 'none' : 'block' }} | ||
| /> |
There was a problem hiding this comment.
Add error handling for image load failures.
The <img> element has an onLoad handler but no onError handler. If the image fails to load (network error, invalid URL, etc.), the loading spinner will display indefinitely with no feedback to the user.
🔎 Suggested error handling
+const [hasError, setHasError] = React.useState(false);
+
+const handleImageError = () => {
+ setIsImageLoading(false);
+ setHasError(true);
+};
<img
src={url}
alt={fileName}
className={mobileView ? classes.previewImageMobile : classes.previewImage}
onLoad={handleImageLoad}
+ onError={handleImageError}
style={{ display: isImageLoading ? 'none' : 'block' }}
/>
+{hasError && <div className={classes.errorMessage}>Failed to load image</div>}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @src/layout/FileUpload/FileUploadTable/ThumbnailPreviewModal.tsx around lines
79-85, ThumbnailPreviewModal currently only uses handleImageLoad on the <img> so
a failed fetch leaves the spinner visible; add an onError handler (e.g.
handleImageError) that sets the same loading state to false and records an error
flag (e.g. setImageLoadError(true)) so the spinner is removed and the UI can
show a fallback (placeholder icon/alt text) or an error message; update the
render logic that currently reads isImageLoading to also check the error flag
and show the fallback UI (or a retry action) instead of the image when
imageLoadError is true.
2 participants
Description
Added metadata to the data element, along with functionality to store the metadata within it.
Additional functionality in app-lib (Altinn/app-lib-dotnet#1598) and localtest(Altinn/app-localtest#183). Storage branch: https://github.com/Altinn/altinn-storage/tree/filippa2
TEST APP: https://altinn.studio/repos/ttd/test-app-filippa
Design in figma, pre development:
Mobileview:
Current layout:
Mobileview:
Preview of thumbnail:
Mobile:
Related Issue(s)
#792
Verification/QA
kind/*andbackport*label to this PR for proper release notes groupingSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.