Skip to content

verification-issue-17.md

Latest commit

 

History

History
86 lines (65 loc) · 2.91 KB

verification-issue-17.md

File metadata and controls

86 lines (65 loc) · 2.91 KB

PR #18 Verification — fix/issue-17-mockup-icons

Verdict

APPROVE ✅

The core bug is fixed: both previously string-literal icon renderings were replaced with real Vue component rendering via v-if/v-else.

1) Diff stat

git diff --stat main...fix/issue-17-mockup-icons
scaffold/spec-site/src/pages/MockupEditorPage.vue | 5 ++++-
scaffold/spec-site/src/pages/MockupListPage.vue   | 5 ++++-
2 files changed, 8 insertions(+), 2 deletions(-)

2) Icon rendering verification (component vs string)

A. MockupEditorPage.vue

  • Before:
    • Used mustache with string literals like:
      • '<Icon name="unlock" :size="14" />'
      • '<Icon name="lock" :size="14" />'
    • This renders text, not a Vue component.
  • After:
    • Replaced with:
      • <Icon v-if="c.props.locked" name="unlock" :size="14" />
      • <Icon v-else name="lock" :size="14" />
  • ✅ Correctly renders real Icon components.

B. MockupListPage.vue

  • Before:
    • {{ m.viewport === 'mobile' ? '📱' : '<Icon name="monitor" :size="14" />' }}
    • Desktop icon branch rendered as text.
  • After:
    • Replaced with:
      • <span v-if="m.viewport === 'mobile'">📱</span>
      • <Icon v-else name="monitor" :size="28" />
  • ✅ Correctly renders Icon component for desktop.

3) File scope check

git diff --name-only main...fix/issue-17-mockup-icons
scaffold/spec-site/src/pages/MockupEditorPage.vue
scaffold/spec-site/src/pages/MockupListPage.vue

✅ Only spec-site files were modified.

4) Potential issues (3+)

  1. Viewport fallback behavior is broad (Low)

    • v-else in MockupListPage.vue treats any non-mobile value as desktop monitor icon.
    • If backend starts sending unexpected values (tablet, unknown, null), UI may silently show incorrect desktop icon.
    • Suggestion: explicit v-else-if="m.viewport === 'desktop'" + fallback icon/text.

  2. Potential visual alignment mismatch (Low)

    • Mobile uses emoji in <span>, desktop uses SVG Icon at fixed size 28.
    • Emoji font metrics differ by OS/browser; cards can appear slightly misaligned.
    • Suggestion: normalize via wrapper style (display:flex; align-items:center; justify-content:center;) and fixed box size.

  3. No regression test coverage in this PR (Low)

    • Template rendering bug was fixed, but no test/assertion added to prevent future regressions.
    • Suggestion: add component test asserting that icon element is present and that raw <Icon ...> text is absent.

  4. Inline conditional complexity can grow (Low)

    • Current v-if/v-else is correct, but repeated template branching for icon selection may spread.
    • Suggestion: move to computed helper (e.g., isLocked, isMobile, viewportIconName) if additional states are introduced.

Final decision

APPROVE — Fix is correct, scope is minimal, and the original rendering bug is resolved without unrelated changes.