Fixups! - #32
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
This PR makes several refinements to the book sticker instruction animation and simplifies the add sighting drawer UI. The changes focus on centralizing animation timing configuration and adjusting the book cover display styling.
Key Changes:
- Introduced centralized animation configuration constants to replace hard-coded timing values
- Modified book container and cover image aspect ratio handling
- Removed the "Register another book" button from the success state
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
components/book-sticker-instruction.tsx |
Adds animation configuration constants, new prop for animation speed multiplier, and modifies aspect ratio handling for the book container and cover image |
components/add-sighting-drawer.tsx |
Removes the "Register another book" button from the generated code success screen |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Derived timings | ||
| const ANIMATION_CONFIG = { | ||
| duration: ANIMATION_BASE_DURATION_S, | ||
| openDelay: ANIMATION_BASE_DURATION_S * 0.3, // ~0.5s if base is 2.5 |
There was a problem hiding this comment.
The comment states "~0.5s if base is 2.5" but the actual calculation is 2.5 * 0.3 = 0.75 seconds, not 0.5 seconds. Update the comment to reflect the correct value.
| openDelay: ANIMATION_BASE_DURATION_S * 0.3, // ~0.5s if base is 2.5 | |
| openDelay: ANIMATION_BASE_DURATION_S * 0.3, // ~0.75s if base is 2.5 |
| const [aspectRatio, setAspectRatio] = useState(1 / 1.3) | ||
|
|
||
| // Refined plan: Use a ref or state for style. | ||
| const [dynamicAspectRatio, setDynamicAspectRatio] = useState(1.3) |
There was a problem hiding this comment.
Two unused state variables are declared: aspectRatio (line 90) and dynamicAspectRatio (line 93). The setDynamicAspectRatio is called in the image onLoad handler but the value is never read. Either use these variables to control the aspect ratio of the container or image, or remove them entirely.
| const [aspectRatio, setAspectRatio] = useState(1 / 1.3) | |
| // Refined plan: Use a ref or state for style. | |
| const [dynamicAspectRatio, setDynamicAspectRatio] = useState(1.3) | |
| // Refined plan: Use a ref or state for style. | |
| const [, setDynamicAspectRatio] = useState(1.3) |
| <div className="flex gap-2 items-center justify-center relative"> | ||
| {/* The 3D Book Container */} | ||
| <div className="relative w-full aspect-[1.3] mt-4 mb-2 max-w-[300px]"> | ||
| <div className="relative w-auto h-[275px] mt-4 mb-2 max-w-[300px] aspect-[0.66]"> |
There was a problem hiding this comment.
The class combination w-auto with aspect-[0.66] creates a conflict. The aspect-[0.66] utility requires a width to calculate height (or vice versa), but w-auto means the width is determined by content. With a fixed height of h-[275px], the aspect ratio class won't work as intended. Consider using only h-[275px] and removing both w-auto and aspect-[0.66], or restructure to use aspect ratio with a defined width.
| <div className="relative w-auto h-[275px] mt-4 mb-2 max-w-[300px] aspect-[0.66]"> | |
| <div className="relative h-[275px] mt-4 mb-2 max-w-[300px] aspect-[0.66]"> |
| src={coverUrl} | ||
| alt="Book Cover" | ||
| className="w-full h-full opacity-90 mix-blend-overlay" | ||
| className="h-full opacity-90 mix-blend-overlay aspect-2/3" |
There was a problem hiding this comment.
The class aspect-2/3 is not a standard Tailwind CSS class. Tailwind uses bracket notation for arbitrary aspect ratios. Change this to aspect-[2/3] to properly apply the 2:3 aspect ratio.
| className="h-full opacity-90 mix-blend-overlay aspect-2/3" | |
| className="h-full opacity-90 mix-blend-overlay aspect-[2/3]" |
| ) : ( | ||
| <div className="space-y-6 text-center animate-in zoom-in-95 duration-500 py-2"> | ||
| <BookStickerInstruction code={generatedCode} coverUrl={selectedBook?.coverUrl} /> |
There was a problem hiding this comment.
The removal of the "Register another book" button leaves the resetState function (defined at line 137) unused and orphaned. Users who want to register multiple books in one session now must close and reopen the drawer, but the generated code and selected book will persist since there's no effect that resets state when the drawer closes. Consider either: 1) Adding a useEffect to reset state when the drawer closes (when open changes to false), or 2) Keeping a reset button if the expected UX is to allow registering multiple books without closing the drawer.
| code: string | ||
| coverUrl?: string | ||
| className?: string | ||
| animationSpeedMultiplier?: number |
There was a problem hiding this comment.
The animationSpeedMultiplier prop is declared in the interface but never destructured or used in the component. Consider removing it if it's not needed, or implement the functionality to use this prop to scale animation timings.
| animationSpeedMultiplier?: number |
No description provided.