Refactor: Replace img-based De Gruyter icons with Svelte icon components #191

New Issue

marcel · 2026-04-07T09:27:23+02:00

marcel commented

2026-04-07 09:27:23 +02:00

Problem

All De Gruyter SVG icons are rendered via <img> tags pointing to /static/degruyter-icons/. Because <img> elements don't inherit CSS color, icons cannot respond to hover/active/focus state changes. This causes:

Edit button on document detail page - icon stays black when button hovers to white background (reported bug)
6 components total have interactive hover states where icons should change color but can't
Dark mode relies on a global filter: invert(1) CSS hack in layout.css instead of proper color inheritance

Proposed solution: Svelte icon components

Convert all 358 SVG files (not just the 22 currently used) into Svelte components with fill="currentColor". Delete the original SVG files afterwards - the Svelte components ARE the icons, no duplication.

Why not a Vite plugin? Plugins like @poppanator/sveltekit-svg generate legacy-mode Svelte components, which adds fragility with Svelte 5 runes. Script-generated components are simpler, fully typed, and zero-dependency.

Why not just fix the SVG source files? Changing fill="#000000" to fill="currentColor" in the SVGs doesn't help because <img> tags don't inherit CSS color regardless. We still need a different rendering method.

Icon set inventory

358 total SVG files across 4 size variants
~152 unique icon names (same icon has different path data per size - not just rescaled)
4 size tiers: XS (12px), SM (16px), MD (24px), LG (32px)
22 currently used in 20 component files
6 components with broken hover/active color states

Architecture

Every icon component has the same API: a size prop ('xs' | 'sm' | 'md' | 'lg'), a class prop, and typed SVG attributes spread onto the <svg>.

If the requested size has dedicated path data in the original icon set, use it. If not, render from the nearest available size and scale via CSS class (h-3 w-3 / h-4 w-4 / h-6 w-6 / h-8 w-8) - the viewBox ensures crisp scaling.

frontend/src/lib/icons/
  IconClose.svelte
  IconArrowLeft.svelte
  IconDownload.svelte
  ...  (~152 components)
  types.ts              (shared IconSize + IconProps interface)
  index.ts              (barrel export)

Shared types (`types.ts`)

import type { SVGAttributes } from 'svelte/elements';

export type IconSize = 'xs' | 'sm' | 'md' | 'lg';

export interface IconProps extends SVGAttributes<SVGSVGElement> {
  size?: IconSize;
  class?: string;
}

This extends SVGAttributes<SVGSVGElement> from svelte/elements, giving consumers full autocomplete for standard SVG attributes (opacity, stroke-width, role, style, etc.).

Example component

<script lang="ts">
  import type { IconProps } from './types.js';

  let { size = 'md', class: className = '', ...rest }: IconProps = $props();

  // Path data for available size variants (from original SVG set)
  const variants: Partial<Record<IconSize, { viewBox: string; d: string }>> = {
    sm: { viewBox: '0 0 16 16', d: 'M14.608...' },
    md: { viewBox: '0 0 24 24', d: 'M3.403...' },
    lg: { viewBox: '0 0 32 32', d: 'M4.571...' },
  };

  const sizeClasses: Record<IconSize, string> = {
    xs: 'h-3 w-3', sm: 'h-4 w-4', md: 'h-6 w-6', lg: 'h-8 w-8'
  };

  // Use dedicated path if available, otherwise fall back to nearest
  const resolved = $derived(variants[size] ?? variants[Object.keys(variants)[0] as IconSize]!);
</script>

<svg
  class="{sizeClasses[size]} {className}"
  viewBox={resolved.viewBox}
  fill="currentColor"
  aria-hidden="true"
  {...rest}
>
  <path d={resolved.d} />
</svg>

Conversion approach

Write a Node.js conversion script (scripts/convert-icons.js) that:

Walks static/degruyter-icons/ recursively
Parses each SVG, extracts: icon name, size tier, viewBox, path/polygon data
Groups by icon name, merges size variants into one component
Generates .svelte files with the uniform IconProps interface
Generates types.ts and index.ts barrel file
Script is deleted after use (one-time conversion)

This avoids hand-converting 358 files and ensures accuracy.

Components currently using icons (20 files)

With broken hover/active states (6):

DocumentTopBar.svelte - Edit, Download, Back arrow buttons
DocumentList.svelte - Arrow-Right turns turquoise on row hover
SearchFilterBar.svelte - Filter chevron and close/reset button
UserMenu.svelte - Account icon opacity/brightness hack
CorrespondenzPersonBar.svelte - Swap and sort arrows
SortDropdown.svelte - Sort direction arrows

Static/decorative icons (14):

DocumentViewer, PersonChipRow, DropZone, ConversationTimeline, PersonCard, PersonDocumentList, PersonsEmptyState, FileSectionEdit, persons/+page, documents/[id]/edit, enrich/+page, enrich/[id]/+page, enrich/done/+page, routes/SearchFilterBar

Implementation plan

Phase 1: Build conversion script

Create scripts/convert-icons.js
Parse all 358 SVGs, extract path data per size variant
Handle both <path> and <polygon> elements (both exist in the set)
Replace fill="#000000" with fill="currentColor"
Generate ~152 Svelte components with uniform IconProps interface + types.ts + index.ts
Review generated output for correctness

Phase 2: Replace img tags across 20 component files

Replace each <img src="/degruyter-icons/..."> with the corresponding <IconName size="md" class="..." /> import
Remove aria-hidden="true" from usage sites (already on the svg in the component)

Phase 3: Remove dark mode CSS hack

Remove filter: invert(1) rules from layout.css
Remove brightness-0 invert utility classes from UserMenu.svelte
Test dark mode across all icon locations

Phase 4: Delete original SVG files + script

Delete static/degruyter-icons/ directory entirely
Delete scripts/convert-icons.js (one-time use)
Remove any remaining references to old paths

Testing

Visual check: all 22 currently-used icons render correctly at their original sizes
Hover states: verify all 6 interactive components show icon color changes
Dark mode: icons follow theme color without CSS filter hacks
Accessibility: aria-hidden="true" still present on decorative icons
Build: no TypeScript or svelte-check regressions

## Problem All De Gruyter SVG icons are rendered via `<img>` tags pointing to `/static/degruyter-icons/`. Because `<img>` elements don't inherit CSS `color`, icons cannot respond to hover/active/focus state changes. This causes: - **Edit button on document detail page** - icon stays black when button hovers to white background (reported bug) - **6 components total** have interactive hover states where icons should change color but can't - Dark mode relies on a global `filter: invert(1)` CSS hack in `layout.css` instead of proper color inheritance ## Proposed solution: Svelte icon components Convert **all 358 SVG files** (not just the 22 currently used) into Svelte components with `fill="currentColor"`. Delete the original SVG files afterwards - the Svelte components ARE the icons, no duplication. **Why not a Vite plugin?** Plugins like `@poppanator/sveltekit-svg` generate legacy-mode Svelte components, which adds fragility with Svelte 5 runes. Script-generated components are simpler, fully typed, and zero-dependency. **Why not just fix the SVG source files?** Changing `fill="#000000"` to `fill="currentColor"` in the SVGs doesn't help because `<img>` tags don't inherit CSS `color` regardless. We still need a different rendering method. ## Icon set inventory - **358 total SVG files** across 4 size variants - **~152 unique icon names** (same icon has different path data per size - not just rescaled) - **4 size tiers**: XS (12px), SM (16px), MD (24px), LG (32px) - **22 currently used** in 20 component files - **6 components** with broken hover/active color states ## Architecture Every icon component has the **same API**: a `size` prop (`'xs' | 'sm' | 'md' | 'lg'`), a `class` prop, and typed SVG attributes spread onto the `<svg>`. If the requested size has dedicated path data in the original icon set, use it. If not, render from the nearest available size and scale via CSS class (`h-3 w-3` / `h-4 w-4` / `h-6 w-6` / `h-8 w-8`) - the viewBox ensures crisp scaling. ``` frontend/src/lib/icons/ IconClose.svelte IconArrowLeft.svelte IconDownload.svelte ... (~152 components) types.ts (shared IconSize + IconProps interface) index.ts (barrel export) ``` ### Shared types (`types.ts`) ```typescript import type { SVGAttributes } from 'svelte/elements'; export type IconSize = 'xs' | 'sm' | 'md' | 'lg'; export interface IconProps extends SVGAttributes<SVGSVGElement> { size?: IconSize; class?: string; } ``` This extends `SVGAttributes<SVGSVGElement>` from `svelte/elements`, giving consumers full autocomplete for standard SVG attributes (`opacity`, `stroke-width`, `role`, `style`, etc.). ### Example component ```svelte <script lang="ts"> import type { IconProps } from './types.js'; let { size = 'md', class: className = '', ...rest }: IconProps = $props(); // Path data for available size variants (from original SVG set) const variants: Partial<Record<IconSize, { viewBox: string; d: string }>> = { sm: { viewBox: '0 0 16 16', d: 'M14.608...' }, md: { viewBox: '0 0 24 24', d: 'M3.403...' }, lg: { viewBox: '0 0 32 32', d: 'M4.571...' }, }; const sizeClasses: Record<IconSize, string> = { xs: 'h-3 w-3', sm: 'h-4 w-4', md: 'h-6 w-6', lg: 'h-8 w-8' }; // Use dedicated path if available, otherwise fall back to nearest const resolved = $derived(variants[size] ?? variants[Object.keys(variants)[0] as IconSize]!); </script> <svg class="{sizeClasses[size]} {className}" viewBox={resolved.viewBox} fill="currentColor" aria-hidden="true" {...rest} > <path d={resolved.d} /> </svg> ``` ## Conversion approach Write a **Node.js conversion script** (`scripts/convert-icons.js`) that: 1. Walks `static/degruyter-icons/` recursively 2. Parses each SVG, extracts: icon name, size tier, viewBox, path/polygon data 3. Groups by icon name, merges size variants into one component 4. Generates `.svelte` files with the uniform `IconProps` interface 5. Generates `types.ts` and `index.ts` barrel file 6. Script is deleted after use (one-time conversion) This avoids hand-converting 358 files and ensures accuracy. ## Components currently using icons (20 files) ### With broken hover/active states (6): 1. **DocumentTopBar.svelte** - Edit, Download, Back arrow buttons 2. **DocumentList.svelte** - Arrow-Right turns turquoise on row hover 3. **SearchFilterBar.svelte** - Filter chevron and close/reset button 4. **UserMenu.svelte** - Account icon opacity/brightness hack 5. **CorrespondenzPersonBar.svelte** - Swap and sort arrows 6. **SortDropdown.svelte** - Sort direction arrows ### Static/decorative icons (14): DocumentViewer, PersonChipRow, DropZone, ConversationTimeline, PersonCard, PersonDocumentList, PersonsEmptyState, FileSectionEdit, persons/+page, documents/[id]/edit, enrich/+page, enrich/[id]/+page, enrich/done/+page, routes/SearchFilterBar ## Implementation plan ### Phase 1: Build conversion script - [ ] Create `scripts/convert-icons.js` - [ ] Parse all 358 SVGs, extract path data per size variant - [ ] Handle both `<path>` and `<polygon>` elements (both exist in the set) - [ ] Replace `fill="#000000"` with `fill="currentColor"` - [ ] Generate ~152 Svelte components with uniform `IconProps` interface + `types.ts` + `index.ts` - [ ] Review generated output for correctness ### Phase 2: Replace img tags across 20 component files - [ ] Replace each `<img src="/degruyter-icons/...">` with the corresponding `<IconName size="md" class="..." />` import - [ ] Remove `aria-hidden="true"` from usage sites (already on the svg in the component) ### Phase 3: Remove dark mode CSS hack - [ ] Remove `filter: invert(1)` rules from `layout.css` - [ ] Remove `brightness-0 invert` utility classes from UserMenu.svelte - [ ] Test dark mode across all icon locations ### Phase 4: Delete original SVG files + script - [ ] Delete `static/degruyter-icons/` directory entirely - [ ] Delete `scripts/convert-icons.js` (one-time use) - [ ] Remove any remaining references to old paths ## Testing - Visual check: all 22 currently-used icons render correctly at their original sizes - Hover states: verify all 6 interactive components show icon color changes - Dark mode: icons follow theme color without CSS filter hacks - Accessibility: `aria-hidden="true"` still present on decorative icons - Build: no TypeScript or svelte-check regressions

marcel added the refactor ui labels 2026-04-07 09:27:28 +02:00

marcel commented

2026-04-07 09:47:51 +02:00

👨‍💻 Felix Brandt — Senior Fullstack Developer

Questions & Observations

Test strategy is underspecified. The issue lists "visual check" and "build: no svelte-check regressions" but doesn't define unit tests for the icon components themselves. Each generated component is code — it should have at least a smoke test proving it renders an <svg> with fill="currentColor" and responds to the size prop. A single parameterized test file iterating over all exports from index.ts would cover this cheaply.
The variants object uses Object.keys()[0] for fallback. Object key order in JS is insertion-order for string keys, but this is an implicit contract. If the conversion script emits sizes in an unexpected order, the fallback silently picks the wrong variant. Consider an explicit fallbackSize constant or an ordered array of preferred fallbacks (['md', 'sm', 'lg', 'xs']).
The example component has Partial<Record<IconSize, ...>> but indexes it with size directly. TypeScript won't narrow this — variants[size] could be undefined. The ?? fallback handles runtime, but the types are loose. A stricter approach: define the available sizes as a union literal per component (generated by the script), so size is constrained to what actually exists.
Barrel file with ~152 re-exports. This is fine for tree-shaking (Vite handles it), but confirm that svelte-check and the dev server don't slow down noticeably. If they do, direct imports (import IconClose from '$lib/icons/IconClose.svelte') without the barrel is the fallback.
aria-hidden="true" hardcoded in every component. This is correct for decorative icons inside labeled buttons, but if an icon is ever used standalone as the only content (e.g., an icon-only button without a visible label), aria-hidden should be overridable. Since {...rest} is spread after, a consumer can override it — worth noting in the implementation.

Suggestions

Add a generated icons.spec.ts that imports every icon from the barrel and renders each at every size. One test file, parameterized, catches regressions for all 152 components.
The conversion script should include a validation step: parse each generated .svelte file back and verify it contains a valid <svg> with at least one <path> or <polygon>.
Consider generating a visual catalog page (/dev/icons or a Storybook-like static page) that renders all icons at all sizes — makes review during Phase 1 much faster than spot-checking individual components.

## 👨‍💻 Felix Brandt — Senior Fullstack Developer ### Questions & Observations - **Test strategy is underspecified.** The issue lists "visual check" and "build: no svelte-check regressions" but doesn't define unit tests for the icon components themselves. Each generated component is code — it should have at least a smoke test proving it renders an `<svg>` with `fill="currentColor"` and responds to the `size` prop. A single parameterized test file iterating over all exports from `index.ts` would cover this cheaply. - **The `variants` object uses `Object.keys()[0]` for fallback.** Object key order in JS is insertion-order for string keys, but this is an implicit contract. If the conversion script emits sizes in an unexpected order, the fallback silently picks the wrong variant. Consider an explicit `fallbackSize` constant or an ordered array of preferred fallbacks (`['md', 'sm', 'lg', 'xs']`). - **The example component has `Partial<Record<IconSize, ...>>` but indexes it with `size` directly.** TypeScript won't narrow this — `variants[size]` could be `undefined`. The `??` fallback handles runtime, but the types are loose. A stricter approach: define the available sizes as a union literal per component (generated by the script), so `size` is constrained to what actually exists. - **Barrel file with ~152 re-exports.** This is fine for tree-shaking (Vite handles it), but confirm that `svelte-check` and the dev server don't slow down noticeably. If they do, direct imports (`import IconClose from '$lib/icons/IconClose.svelte'`) without the barrel is the fallback. - **`aria-hidden="true"` hardcoded in every component.** This is correct for decorative icons inside labeled buttons, but if an icon is ever used standalone as the only content (e.g., an icon-only button without a visible label), `aria-hidden` should be overridable. Since `{...rest}` is spread after, a consumer can override it — worth noting in the implementation. ### Suggestions - Add a generated `icons.spec.ts` that imports every icon from the barrel and renders each at every size. One test file, parameterized, catches regressions for all 152 components. - The conversion script should include a validation step: parse each generated `.svelte` file back and verify it contains a valid `<svg>` with at least one `<path>` or `<polygon>`. - Consider generating a visual catalog page (`/dev/icons` or a Storybook-like static page) that renders all icons at all sizes — makes review during Phase 1 much faster than spot-checking individual components.

marcel commented

2026-04-07 09:48:14 +02:00

🏗️ Markus Keller — Application Architect

Questions & Observations

152 components is a lot of generated code to own. The conversion script runs once and gets deleted, meaning future maintenance (adding a new icon, fixing a path) is manual editing of generated Svelte files. If De Gruyter ships an updated icon set, you'd need to re-create the script or manually diff 152 files. Consider keeping the script in the repo (perhaps in scripts/ with a note that it's re-runnable) so the conversion is repeatable. One-time scripts have a habit of being needed twice.
The $lib/icons/ directory becomes a de facto icon library. 152 components is a module boundary worth thinking about. Will these ever be shared across projects? If not, $lib/icons/ is fine. If yes, consider whether this should eventually be an internal package. For now, the flat directory is the right call — just flag it as a potential extraction boundary.
sizeClasses hardcodes Tailwind values inside each component. This couples every icon to Tailwind's utility class names. If the project ever changes its sizing scale (unlikely but possible), you'd update 152 files. An alternative: move sizeClasses into types.ts as a shared constant. One definition, 152 consumers.
Fallback behavior when a size variant doesn't exist. The issue says "scale from the nearest available size via CSS class." This is a sensible default, but the consumer gets no signal that they're using a scaled fallback vs. a native variant. Is that acceptable? For this project, yes — the visual difference between a scaled 24px icon at 16px and a native 16px icon is minor. Just be aware of it.
Phase 4 deletes the entire static/degruyter-icons/ directory. This is a one-way door. Before deleting, verify no other consumer references these files — check layout.css (the filter: invert rule uses img[src*='degruyter-icons']), any HTML email templates, or external tools that might hotlink to static assets.

Suggestions

Keep scripts/convert-icons.js in the repo, gitignored or in a scripts/ directory, so the conversion is repeatable if the icon set is updated.
Extract sizeClasses into the shared types.ts module to avoid 152 copies of the same mapping.
Add a brief ADR note (even just a paragraph in the PR description) explaining why Svelte components over plugin/mask-image/raw-import — this decision will be questioned by future contributors.

## 🏗️ Markus Keller — Application Architect ### Questions & Observations - **152 components is a lot of generated code to own.** The conversion script runs once and gets deleted, meaning future maintenance (adding a new icon, fixing a path) is manual editing of generated Svelte files. If De Gruyter ships an updated icon set, you'd need to re-create the script or manually diff 152 files. Consider keeping the script in the repo (perhaps in `scripts/` with a note that it's re-runnable) so the conversion is repeatable. One-time scripts have a habit of being needed twice. - **The `$lib/icons/` directory becomes a de facto icon library.** 152 components is a module boundary worth thinking about. Will these ever be shared across projects? If not, `$lib/icons/` is fine. If yes, consider whether this should eventually be an internal package. For now, the flat directory is the right call — just flag it as a potential extraction boundary. - **`sizeClasses` hardcodes Tailwind values inside each component.** This couples every icon to Tailwind's utility class names. If the project ever changes its sizing scale (unlikely but possible), you'd update 152 files. An alternative: move `sizeClasses` into `types.ts` as a shared constant. One definition, 152 consumers. - **Fallback behavior when a size variant doesn't exist.** The issue says "scale from the nearest available size via CSS class." This is a sensible default, but the consumer gets no signal that they're using a scaled fallback vs. a native variant. Is that acceptable? For this project, yes — the visual difference between a scaled 24px icon at 16px and a native 16px icon is minor. Just be aware of it. - **Phase 4 deletes the entire `static/degruyter-icons/` directory.** This is a one-way door. Before deleting, verify no other consumer references these files — check `layout.css` (the `filter: invert` rule uses `img[src*='degruyter-icons']`), any HTML email templates, or external tools that might hotlink to static assets. ### Suggestions - Keep `scripts/convert-icons.js` in the repo, gitignored or in a `scripts/` directory, so the conversion is repeatable if the icon set is updated. - Extract `sizeClasses` into the shared `types.ts` module to avoid 152 copies of the same mapping. - Add a brief ADR note (even just a paragraph in the PR description) explaining why Svelte components over plugin/mask-image/raw-import — this decision will be questioned by future contributors.

marcel commented

2026-04-07 09:48:29 +02:00

🧪 Sara Holt — QA Engineer

Questions & Observations

No acceptance criteria for visual correctness. "Visual check: all 22 icons render correctly" is not testable in CI. How do we know an icon converted correctly? The conversion script could silently produce a broken path (e.g., polygon points not converted to a d attribute properly). We need an automated verification step.
Polygon-to-path conversion is a risk area. The issue notes both <path> and <polygon> elements exist in the SVG set. The example component only shows <path d={...}>. How will <polygon points="..."> be handled? If converted to <path>, the conversion logic needs testing. If rendered as <polygon>, the component template needs to support both element types. Either way, this needs explicit test coverage.
Dark mode regression risk is high. Phase 3 removes the global filter: invert(1) CSS hack. If even one icon is missed during Phase 2 replacement (still using <img>) and the CSS hack is already removed, that icon becomes invisible in dark mode. Suggestion: Phase 2 and Phase 3 should be verified together — don't remove the CSS hack until a grep confirms zero remaining <img src="...degruyter-icons..." references.
The 6 hover-state components need explicit before/after verification. Each of the 6 components listed should have a test (at minimum a Playwright visual snapshot) confirming the icon color changes on hover. This is the original bug — it should be the primary acceptance criterion.

Suggestions

Conversion script validation: after generating all components, the script should render each one (or at least parse the SVG markup) and compare the path data against the source SVG. A simple string-equality check on the d attribute catches most conversion bugs.
Add a CI step: grep -r "degruyter-icons" frontend/src/ should return zero matches after Phase 2. This prevents partial migration.
Parameterized smoke test: as Felix suggested, one test file that imports all icons and renders each at each size. Assert: <svg> element exists, has fill="currentColor", has at least one child element. Fast, catches regressions on all 152 components.
Phase ordering safeguard: don't merge Phase 3 (remove CSS hack) until Phase 2 (replace all img tags) is verified complete. Consider doing them in the same commit to avoid a broken intermediate state.

## 🧪 Sara Holt — QA Engineer ### Questions & Observations - **No acceptance criteria for visual correctness.** "Visual check: all 22 icons render correctly" is not testable in CI. How do we know an icon converted correctly? The conversion script could silently produce a broken path (e.g., polygon points not converted to a `d` attribute properly). We need an automated verification step. - **Polygon-to-path conversion is a risk area.** The issue notes both `<path>` and `<polygon>` elements exist in the SVG set. The example component only shows `<path d={...}>`. How will `<polygon points="...">` be handled? If converted to `<path>`, the conversion logic needs testing. If rendered as `<polygon>`, the component template needs to support both element types. Either way, this needs explicit test coverage. - **Dark mode regression risk is high.** Phase 3 removes the global `filter: invert(1)` CSS hack. If even one icon is missed during Phase 2 replacement (still using `<img>`) and the CSS hack is already removed, that icon becomes invisible in dark mode. Suggestion: Phase 2 and Phase 3 should be verified together — don't remove the CSS hack until a grep confirms zero remaining `<img src="...degruyter-icons..."` references. - **The 6 hover-state components need explicit before/after verification.** Each of the 6 components listed should have a test (at minimum a Playwright visual snapshot) confirming the icon color changes on hover. This is the original bug — it should be the primary acceptance criterion. ### Suggestions - **Conversion script validation**: after generating all components, the script should render each one (or at least parse the SVG markup) and compare the path data against the source SVG. A simple string-equality check on the `d` attribute catches most conversion bugs. - **Add a CI step**: `grep -r "degruyter-icons" frontend/src/` should return zero matches after Phase 2. This prevents partial migration. - **Parameterized smoke test**: as Felix suggested, one test file that imports all icons and renders each at each size. Assert: `<svg>` element exists, has `fill="currentColor"`, has at least one child element. Fast, catches regressions on all 152 components. - **Phase ordering safeguard**: don't merge Phase 3 (remove CSS hack) until Phase 2 (replace all img tags) is verified complete. Consider doing them in the same commit to avoid a broken intermediate state.

marcel commented

2026-04-07 09:48:56 +02:00

🔒 Nora "NullX" Steiner — Application Security Engineer

Questions & Observations

{@html} is not used — good. The proposed approach generates static Svelte markup with hardcoded path data, not runtime-injected SVG strings. No XSS surface from the icon system itself. This is the correct approach.
{...rest} spread on <svg> is safe but worth noting. Spreading arbitrary props onto an SVG element is low-risk (SVG attributes can't execute scripts the way innerHTML can), but a consumer could theoretically pass onload or event handler props. In Svelte 5, event handlers via props are standard, so this is expected behavior — not a vulnerability, just something to be aware of.
The conversion script parses untrusted SVG files. The De Gruyter icon set is a trusted first-party source, but the script still parses XML. If using a DOM parser (e.g., jsdom, cheerio), ensure it doesn't resolve external entities (XXE). Node.js DOMParser and cheerio are safe by default, but if using libxmljs or similar, disable entity resolution explicitly. Since this is a one-time local script and not a production endpoint, the risk is informational-level.
Deleting static assets changes the attack surface slightly. Currently, static/degruyter-icons/ serves ~358 SVG files to any client. These are inert, but they increase the static file footprint. Removing them is a minor positive from a surface reduction perspective.

Suggestions

No security concerns that would block this work. The approach is sound — static markup, no runtime injection, no user-controlled paths.
If the conversion script uses any XML parsing library, add a one-line comment noting that external entity resolution is disabled (or that the parser is safe by default). This is documentation hygiene, not a critical fix.

## 🔒 Nora "NullX" Steiner — Application Security Engineer ### Questions & Observations - **`{@html}` is not used — good.** The proposed approach generates static Svelte markup with hardcoded path data, not runtime-injected SVG strings. No XSS surface from the icon system itself. This is the correct approach. - **`{...rest}` spread on `<svg>` is safe but worth noting.** Spreading arbitrary props onto an SVG element is low-risk (SVG attributes can't execute scripts the way `innerHTML` can), but a consumer could theoretically pass `onload` or event handler props. In Svelte 5, event handlers via props are standard, so this is expected behavior — not a vulnerability, just something to be aware of. - **The conversion script parses untrusted SVG files.** The De Gruyter icon set is a trusted first-party source, but the script still parses XML. If using a DOM parser (e.g., `jsdom`, `cheerio`), ensure it doesn't resolve external entities (XXE). Node.js `DOMParser` and `cheerio` are safe by default, but if using `libxmljs` or similar, disable entity resolution explicitly. Since this is a one-time local script and not a production endpoint, the risk is informational-level. - **Deleting static assets changes the attack surface slightly.** Currently, `static/degruyter-icons/` serves ~358 SVG files to any client. These are inert, but they increase the static file footprint. Removing them is a minor positive from a surface reduction perspective. ### Suggestions - No security concerns that would block this work. The approach is sound — static markup, no runtime injection, no user-controlled paths. - If the conversion script uses any XML parsing library, add a one-line comment noting that external entity resolution is disabled (or that the parser is safe by default). This is documentation hygiene, not a critical fix.

marcel commented

2026-04-07 09:49:05 +02:00

🎨 Leonie Voss — UI/UX Design Lead

Questions & Observations

This directly fixes the reported bug and five other hover-state issues. The edit icon on the document detail page not responding to hover is a real usability problem — users get no visual feedback that the button is interactive. fill="currentColor" is the correct fix. Full support for this approach.
The sizeClasses mapping needs scrutiny. The proposed mapping is xs: h-3 w-3 (12px), sm: h-4 w-4 (16px), md: h-6 w-6 (24px), lg: h-8 w-8 (32px). But the current codebase uses h-5 w-5 (20px) for most icons — check the DocumentTopBar, SearchFilterBar, and other components. If md maps to h-6 w-6 (24px) but every usage site currently uses h-5 w-5, consumers will need to override with class="h-5 w-5" every time, defeating the purpose of the size prop. Consider whether the size prop should map to the actual icon viewBox sizes or to common usage sizes.
Touch targets are not affected. The icons themselves are decorative elements inside larger interactive containers (buttons, links). The refactor changes rendering method, not layout. Touch targets remain determined by the parent element's padding. No regression expected here.
Dark mode improvement is significant. The current filter: invert(1) hack inverts the entire SVG bitmap, which can produce artifacts on icons with subtle opacity or anti-aliasing. currentColor inheritance via CSS custom properties (which this project already uses for dark mode) will produce cleaner, crisper icons in dark mode. This is an underappreciated benefit of the refactor.
aria-hidden="true" default is correct for this project's usage. Every icon currently sits inside a button or link with a visible text label or an aria-label. The icon is decorative. However, the {...rest} spread allows overriding to aria-hidden="false" plus adding a role="img" and aria-label for any future standalone icon usage — good escape hatch.

Suggestions

Verify the sizeClasses mapping against actual current usage. If h-5 w-5 is the dominant size, consider adding it as a named size (e.g., 'default' or adjusting md to 20px).
After Phase 2, do a visual diff of every page that uses icons in both light and dark mode. The dark mode improvement is real but needs confirmation that no icon looks wrong with currentColor (e.g., if any icon relied on the inversion producing a specific shade rather than pure white/black).

## 🎨 Leonie Voss — UI/UX Design Lead ### Questions & Observations - **This directly fixes the reported bug and five other hover-state issues.** The edit icon on the document detail page not responding to hover is a real usability problem — users get no visual feedback that the button is interactive. `fill="currentColor"` is the correct fix. Full support for this approach. - **The `sizeClasses` mapping needs scrutiny.** The proposed mapping is `xs: h-3 w-3` (12px), `sm: h-4 w-4` (16px), `md: h-6 w-6` (24px), `lg: h-8 w-8` (32px). But the current codebase uses `h-5 w-5` (20px) for most icons — check the DocumentTopBar, SearchFilterBar, and other components. If `md` maps to `h-6 w-6` (24px) but every usage site currently uses `h-5 w-5`, consumers will need to override with `class="h-5 w-5"` every time, defeating the purpose of the size prop. Consider whether the size prop should map to the actual icon viewBox sizes or to common usage sizes. - **Touch targets are not affected.** The icons themselves are decorative elements inside larger interactive containers (buttons, links). The refactor changes rendering method, not layout. Touch targets remain determined by the parent element's padding. No regression expected here. - **Dark mode improvement is significant.** The current `filter: invert(1)` hack inverts the entire SVG bitmap, which can produce artifacts on icons with subtle opacity or anti-aliasing. `currentColor` inheritance via CSS custom properties (which this project already uses for dark mode) will produce cleaner, crisper icons in dark mode. This is an underappreciated benefit of the refactor. - **`aria-hidden="true"` default is correct for this project's usage.** Every icon currently sits inside a button or link with a visible text label or an `aria-label`. The icon is decorative. However, the `{...rest}` spread allows overriding to `aria-hidden="false"` plus adding a `role="img"` and `aria-label` for any future standalone icon usage — good escape hatch. ### Suggestions - Verify the `sizeClasses` mapping against actual current usage. If `h-5 w-5` is the dominant size, consider adding it as a named size (e.g., `'default'` or adjusting `md` to 20px). - After Phase 2, do a visual diff of every page that uses icons in both light and dark mode. The dark mode improvement is real but needs confirmation that no icon looks wrong with `currentColor` (e.g., if any icon relied on the inversion producing a specific shade rather than pure white/black).

marcel commented

2026-04-07 09:49:22 +02:00

🔧 Tobias Wendt — DevOps & Platform Engineer

Questions & Observations

Build size impact. 152 Svelte components replacing 358 static SVG files. The static files are served as-is (no processing), while Svelte components get compiled into JS. For the 22 currently used icons, the compiled output should be smaller than loading external SVGs via <img> (no HTTP requests). The 130 unused icons will be tree-shaken by Vite — they won't appear in the production bundle. Net effect: likely a smaller production build. Worth verifying with npm run build before/after comparison.
CI pipeline impact is minimal. The conversion script runs once locally, not in CI. The generated .svelte files are committed like any other source code. No new CI step needed. svelte-check and Vitest will cover the generated components as part of the existing pipeline.
No infrastructure or deployment changes. This is a pure frontend refactor — no new services, no config changes, no environment variables. The static/degruyter-icons/ deletion reduces the Docker image size slightly (358 SVGs removed from the static asset layer).
Cache invalidation is handled by SvelteKit. The old <img> tags referenced static paths that could be browser-cached indefinitely. After the migration, icons are inlined in the compiled JS bundle, which SvelteKit fingerprints with content hashes. No stale-cache risk for users after deployment.

Suggestions

No concerns from the infrastructure angle. This refactor is entirely within the frontend build boundary — no deployment, config, or runtime impact.
After the migration, verify the production Docker image size decreased (or at least didn't increase meaningfully) by comparing docker images output before and after.

## 🔧 Tobias Wendt — DevOps & Platform Engineer ### Questions & Observations - **Build size impact.** 152 Svelte components replacing 358 static SVG files. The static files are served as-is (no processing), while Svelte components get compiled into JS. For the 22 currently used icons, the compiled output should be smaller than loading external SVGs via `<img>` (no HTTP requests). The 130 unused icons will be tree-shaken by Vite — they won't appear in the production bundle. Net effect: likely a smaller production build. Worth verifying with `npm run build` before/after comparison. - **CI pipeline impact is minimal.** The conversion script runs once locally, not in CI. The generated `.svelte` files are committed like any other source code. No new CI step needed. `svelte-check` and Vitest will cover the generated components as part of the existing pipeline. - **No infrastructure or deployment changes.** This is a pure frontend refactor — no new services, no config changes, no environment variables. The `static/degruyter-icons/` deletion reduces the Docker image size slightly (358 SVGs removed from the static asset layer). - **Cache invalidation is handled by SvelteKit.** The old `<img>` tags referenced static paths that could be browser-cached indefinitely. After the migration, icons are inlined in the compiled JS bundle, which SvelteKit fingerprints with content hashes. No stale-cache risk for users after deployment. ### Suggestions - No concerns from the infrastructure angle. This refactor is entirely within the frontend build boundary — no deployment, config, or runtime impact. - After the migration, verify the production Docker image size decreased (or at least didn't increase meaningfully) by comparing `docker images` output before and after.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: marcel/familienarchiv#191

Refactor: Replace img-based De Gruyter icons with Svelte icon components #191

Problem

Proposed solution: Svelte icon components

Icon set inventory

Architecture

Shared types (types.ts)

Example component

Conversion approach

Components currently using icons (20 files)

With broken hover/active states (6):

Static/decorative icons (14):

Implementation plan

Phase 1: Build conversion script

Phase 2: Replace img tags across 20 component files

Phase 3: Remove dark mode CSS hack

Phase 4: Delete original SVG files + script

Testing

👨‍💻 Felix Brandt — Senior Fullstack Developer

Questions & Observations

Suggestions

🏗️ Markus Keller — Application Architect

Questions & Observations

Suggestions

🧪 Sara Holt — QA Engineer

Questions & Observations

Suggestions

🔒 Nora "NullX" Steiner — Application Security Engineer

Questions & Observations

Suggestions

🎨 Leonie Voss — UI/UX Design Lead

Questions & Observations

Suggestions

🔧 Tobias Wendt — DevOps & Platform Engineer

Questions & Observations

Suggestions

Shared types (`types.ts`)