-
Notifications
You must be signed in to change notification settings - Fork 0
Report list photos, image lightbox, announcement label (#118) #22
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
Changes from all commits
3bf20b5
de08c55
584627c
4d288c3
dc955c6
30609e7
94c2daa
fd48476
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| import { describe, it, expect } from "vitest" | ||
|
|
||
| import { startIndex, stepIndex } from "@/components/shared/lightbox" | ||
|
|
||
| describe("lightbox paging", () => { | ||
| it("wraps forward past the last photo and backward past the first", () => { | ||
| expect(stepIndex(0, 1, 3)).toBe(1) | ||
| expect(stepIndex(2, 1, 3)).toBe(0) | ||
| expect(stepIndex(0, -1, 3)).toBe(2) | ||
| expect(stepIndex(1, -1, 3)).toBe(0) | ||
| }) | ||
|
|
||
| it("stays put on a single photo and never divides by an empty set", () => { | ||
| expect(stepIndex(0, 1, 1)).toBe(0) | ||
| expect(stepIndex(0, -1, 1)).toBe(0) | ||
| expect(stepIndex(0, 1, 0)).toBe(0) | ||
| }) | ||
|
|
||
| it("clamps an out-of-range start to the first photo", () => { | ||
| expect(startIndex(2, 4)).toBe(2) | ||
| expect(startIndex(-1, 4)).toBe(0) | ||
| expect(startIndex(4, 4)).toBe(0) | ||
| }) | ||
| }) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,177 @@ | ||
| "use client" | ||
|
|
||
| import * as React from "react" | ||
| import { create } from "zustand" | ||
|
|
||
| import { Icons } from "@/components/icons" | ||
| import { useModalFocus } from "@/components/shared/modal-focus" | ||
|
|
||
| export interface LightboxImage { | ||
| id: string | ||
| url: string | ||
| alt: string | ||
| } | ||
|
|
||
| export function stepIndex(index: number, delta: number, count: number): number { | ||
| if (count <= 0) return 0 | ||
| return (((index + delta) % count) + count) % count | ||
| } | ||
|
|
||
| export function startIndex(index: number, count: number): number { | ||
| return index >= 0 && index < count ? index : 0 | ||
| } | ||
|
|
||
| interface LightboxState { | ||
| images: LightboxImage[] | ||
| index: number | ||
| refresh: (() => void) | null | ||
| open: (images: LightboxImage[], index: number, refresh: (() => void) | null) => void | ||
| close: () => void | ||
| step: (delta: number) => void | ||
| sync: (images: LightboxImage[]) => void | ||
| } | ||
|
|
||
| const useLightboxStore = create<LightboxState>((set) => ({ | ||
| images: [], | ||
| index: 0, | ||
| refresh: null, | ||
| open: (images, index, refresh) => set({ images, index, refresh }), | ||
| close: () => set({ images: [], index: 0, refresh: null }), | ||
| step: (delta) => | ||
| set((s) => (s.images.length === 0 ? s : { index: stepIndex(s.index, delta, s.images.length) })), | ||
| sync: (images) => | ||
| set((s) => { | ||
| if (s.images.length === 0) return s | ||
| const next = s.images.map((shown) => images.find((i) => i.id === shown.id) ?? shown) | ||
| return next.every((img, i) => img.url === s.images[i]?.url) ? s : { images: next } | ||
| }), | ||
| })) | ||
|
|
||
| export function openLightbox(images: LightboxImage[], index = 0, refresh?: () => void): void { | ||
| if (images.length === 0) return | ||
| useLightboxStore.getState().open(images, startIndex(index, images.length), refresh ?? null) | ||
| } | ||
|
|
||
| export function LightboxSync({ images }: { images: LightboxImage[] }) { | ||
| React.useEffect(() => { | ||
| useLightboxStore.getState().sync(images) | ||
| }, [images]) | ||
| return null | ||
| } | ||
|
|
||
| export function LightboxHost() { | ||
| const images = useLightboxStore((s) => s.images) | ||
| const index = useLightboxStore((s) => s.index) | ||
| const refresh = useLightboxStore((s) => s.refresh) | ||
| const close = useLightboxStore((s) => s.close) | ||
| const step = useLightboxStore((s) => s.step) | ||
| const count = images.length | ||
| const frameRef = useModalFocus<HTMLDivElement>(count > 0) | ||
| const current = count > 0 ? (images[index] ?? images[0]) : undefined | ||
| const url = current?.url ?? null | ||
| const [load, setLoad] = React.useState<{ | ||
| url: string | null | ||
| status: "loading" | "ready" | "failed" | ||
| attempt: number | ||
| }>({ url: null, status: "loading", attempt: 0 }) | ||
| const forCurrent = load.url === url | ||
| const status = forCurrent ? load.status : "loading" | ||
| const attempt = forCurrent ? load.attempt : 0 | ||
|
|
||
| React.useEffect(() => { | ||
| if (count === 0) return | ||
| const onKey = (e: KeyboardEvent) => { | ||
| if (e.key === "Escape") { | ||
| e.preventDefault() | ||
| close() | ||
| } else if (e.key === "ArrowRight" && count > 1) { | ||
| e.preventDefault() | ||
| step(1) | ||
| } else if (e.key === "ArrowLeft" && count > 1) { | ||
| e.preventDefault() | ||
| step(-1) | ||
| } | ||
| } | ||
| window.addEventListener("keydown", onKey) | ||
| return () => window.removeEventListener("keydown", onKey) | ||
| }, [count, close, step]) | ||
|
|
||
| if (!current) return null | ||
| const many = count > 1 | ||
|
|
||
| const retry = () => { | ||
| setLoad({ url: current.url, status: "loading", attempt: attempt + 1 }) | ||
| refresh?.() | ||
| } | ||
|
|
||
| return ( | ||
| <div className="modal-overlay lightbox-overlay" onClick={close}> | ||
| <div | ||
| ref={frameRef} | ||
| className="lightbox" | ||
| onClick={(e) => e.stopPropagation()} | ||
| role="dialog" | ||
| aria-modal="true" | ||
| aria-label={current.alt} | ||
| > | ||
| <div className="lightbox-bar"> | ||
| {many && ( | ||
| <span className="lightbox-count mono"> | ||
| {index + 1} / {count} | ||
| </span> | ||
| )} | ||
| <button className="lightbox-btn" onClick={close} aria-label="Close"> | ||
| <Icons.X size={16} /> | ||
| </button> | ||
| </div> | ||
| {status !== "failed" && ( | ||
| // eslint-disable-next-line @next/next/no-img-element | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The new lightbox adds an inline lint-suppression comment, and the new report chat image rendering adds the same kind of comment. This violates the directive that new code contain no comments. Use an implementation or project configuration that does not require new inline comments; this repository requirement must be satisfied before merging. Rule Used: # civfix review rules civfix is a live civic-tech platform that will hold government contracts. Review every PR for correctness, security and performance. Flag real defects with evidence; skip style nits that lint already covers. ## Repos - **... (source) Prompt To Fix With AIThis is a comment left during a code review.
Path: apps/admin/src/components/shared/lightbox.tsx
Line: 128
Comment:
**Remove new code comments**
The new lightbox adds an inline lint-suppression comment, and the new report chat image rendering adds the same kind of comment. This violates the directive that new code contain no comments. Use an implementation or project configuration that does not require new inline comments; this repository requirement must be satisfied before merging.
**Rule Used:** # civfix review rules civfix is a live civic-tech platform that will hold government contracts. Review every PR for **correctness, security and performance**. Flag real defects with evidence; skip style nits that lint already covers. ## Repos - **... ([source](https://app.greptile.com/civfix/-/custom-context?memory=39a53925-3d93-4c82-980e-27b67393717d))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! |
||
| <img | ||
| key={`${current.id}:${attempt}`} | ||
| className={`lightbox-img ${status === "ready" ? "" : "pending"}`} | ||
| src={current.url} | ||
| alt={current.alt} | ||
| decoding="async" | ||
| onLoad={() => setLoad({ url: current.url, status: "ready", attempt })} | ||
| onError={() => setLoad({ url: current.url, status: "failed", attempt })} | ||
| /> | ||
| )} | ||
| {status === "loading" && ( | ||
| <div className="lightbox-face" role="status" aria-live="polite"> | ||
| <span className="op-spin" aria-hidden="true" /> | ||
| <span>Loading photo...</span> | ||
| </div> | ||
| )} | ||
| {status === "failed" && ( | ||
| <div className="lightbox-face" role="alert"> | ||
| <span className="lightbox-face-title">This photo link expired</span> | ||
| <span className="lightbox-face-sub"> | ||
| Photo links are short-lived. Refresh to fetch a new one. | ||
| </span> | ||
| <button type="button" className="lightbox-face-btn" onClick={retry}> | ||
| Refresh photo | ||
| </button> | ||
| </div> | ||
| )} | ||
| {many && ( | ||
| <> | ||
| <button | ||
| className="lightbox-btn lightbox-prev" | ||
| onClick={() => step(-1)} | ||
| aria-label="Previous photo" | ||
| > | ||
| <Icons.ChevronLeft size={20} /> | ||
| </button> | ||
| <button | ||
| className="lightbox-btn lightbox-next" | ||
| onClick={() => step(1)} | ||
| aria-label="Next photo" | ||
| > | ||
| <Icons.ChevronRight size={20} /> | ||
| </button> | ||
| </> | ||
| )} | ||
| </div> | ||
| </div> | ||
| ) | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| import { readFileSync } from "node:fs" | ||
|
|
||
| import { describe, expect, it } from "vitest" | ||
|
|
||
| import { ESCAPE_OWNER_SELECTOR } from "@/components/shell/escape-owner" | ||
|
|
||
| const lightboxSource = readFileSync(new URL("./lightbox.tsx", import.meta.url), "utf8") | ||
| const dialogSource = readFileSync(new URL("./dialog.tsx", import.meta.url), "utf8") | ||
|
|
||
| const lightboxFrame = | ||
| lightboxSource.match(/className="lightbox"[\s\S]{0,300}?aria-label=\{current\.alt\}/)?.[0] ?? "" | ||
|
|
||
| describe("modal host markup", () => { | ||
| it("marks the lightbox frame as the modal dialog the shell yields Escape to", () => { | ||
| expect(ESCAPE_OWNER_SELECTOR).toContain('[role="dialog"][aria-modal="true"]') | ||
| expect(lightboxFrame).toMatch(/role="dialog"/) | ||
| expect(lightboxFrame).toMatch(/aria-modal="true"/) | ||
| expect(lightboxFrame).not.toMatch(/aria-hidden/) | ||
| }) | ||
|
|
||
| it("traps and restores focus in both hosts through the shared helper", () => { | ||
| for (const source of [lightboxSource, dialogSource]) { | ||
| expect(source).toMatch(/useModalFocus<HTMLDivElement>\(/) | ||
| expect(source).toMatch(/ref=\{(frameRef|modalRef)\}/) | ||
| } | ||
| }) | ||
|
|
||
| it("keeps an explicit outcome for a photo that fails to load", () => { | ||
| expect(lightboxSource).toMatch(/onError=/) | ||
| expect(lightboxSource).toMatch(/onLoad=/) | ||
| expect(lightboxSource).toContain("Refresh photo") | ||
| }) | ||
| }) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,47 @@ | ||
| "use client" | ||
|
|
||
| import * as React from "react" | ||
|
|
||
| const FOCUSABLE_SELECTOR = | ||
| 'a[href], button:not([disabled]), input:not([disabled]), textarea:not([disabled]), select:not([disabled]), [tabindex]:not([tabindex="-1"])' | ||
|
|
||
| function focusableWithin(container: HTMLElement): HTMLElement[] { | ||
| return [...container.querySelectorAll<HTMLElement>(FOCUSABLE_SELECTOR)] | ||
| } | ||
|
|
||
| export function useModalFocus<T extends HTMLElement>(open: boolean): React.RefObject<T | null> { | ||
| const ref = React.useRef<T>(null) | ||
|
|
||
| React.useEffect(() => { | ||
| const container = ref.current | ||
| if (!open || !container) return | ||
| const restoreTo = document.activeElement as HTMLElement | null | ||
|
|
||
| if (!container.contains(document.activeElement)) focusableWithin(container)[0]?.focus() | ||
|
|
||
| const onKey = (e: KeyboardEvent) => { | ||
| if (e.key !== "Tab") return | ||
| const focusable = focusableWithin(container) | ||
| const first = focusable[0] | ||
| const last = focusable[focusable.length - 1] | ||
| if (!first || !last) { | ||
| e.preventDefault() | ||
| return | ||
| } | ||
| const active = document.activeElement | ||
| const leavingBackwards = e.shiftKey && (active === first || !container.contains(active)) | ||
| const leavingForwards = !e.shiftKey && (active === last || !container.contains(active)) | ||
| if (!leavingBackwards && !leavingForwards) return | ||
| e.preventDefault() | ||
| ;(e.shiftKey ? last : first).focus() | ||
| } | ||
|
|
||
| document.addEventListener("keydown", onKey, true) | ||
| return () => { | ||
| document.removeEventListener("keydown", onKey, true) | ||
| if (restoreTo && document.contains(restoreTo)) restoreTo.focus() | ||
| } | ||
| }, [open]) | ||
|
|
||
| return ref | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change exceeds the repository's 400-line pull-request limit, but its description does not include the required
Size:justification. Add the explicit justification or split the modal accessibility, report-photo viewer, and announcement-label work into smaller changes; this repository requirement must be satisfied before merging.Rule Used: # civfix review rules civfix is a live civic-tech platform that will hold government contracts. Review every PR for correctness, security and performance. Flag real defects with evidence; skip style nits that lint already covers. ## Repos - **... (source)
Prompt To Fix With AI
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!