-
Notifications
You must be signed in to change notification settings - Fork 21
Impleneted simple react photo editor according to spec #11
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
base: react/photo-editor
Are you sure you want to change the base?
Impleneted simple react photo editor according to spec #11
Conversation
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.
Pull Request Overview
This PR implements a React-based photo editor application with a maintainable architecture using Feature-Sliced Design (FSD). The implementation includes photo upload functionality, canvas-based image manipulation, JSON configuration import/export, and comprehensive testing infrastructure.
Key changes:
- Complete rewrite of the photo editor with FSD architecture and context-based state management
- Migration from Parcel to Vite build system with updated React 19 and modern testing tools
- Addition of BiomeJS for linting/formatting and Husky for pre-push validation
Reviewed Changes
Copilot reviewed 25 out of 28 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| package.json | Updated build tools (Vite), testing libraries (Vitest), React 19, and added Zod validation |
| vite.config.ts | New Vite configuration with path aliases and test setup |
| app/index.tsx | Wrapped PhotoEditor with PrintDescriptionProvider context |
| app/features/photo-editor/components/photo-editor/index.tsx | Complete photo editor implementation with upload, move, and export features |
| app/features/photo-editor/contexts/printDescriptionContext.tsx | Context provider for managing canvas and photo state |
| app/features/photo-editor/hooks/useDrawCanvasImage.ts | Custom hook for canvas image rendering with aspect ratio handling |
| app/helpers/*.ts | Utility functions for validation, unit conversion, file download, and image conversion |
| biome.json | BiomeJS configuration for code quality |
| tests/* | Test setup and placeholder integration tests |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| export default defineConfig({ | ||
| plugins: [require('@vitejs/plugin-react')], |
Copilot
AI
Oct 27, 2025
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.
Using require() in an ES module file is inconsistent. Import the plugin using ES6 syntax: import react from '@vitejs/plugin-react' and use plugins: [react()].
| export default defineConfig({ | |
| plugins: [require('@vitejs/plugin-react')], | |
| import react from '@vitejs/plugin-react' | |
| export default defineConfig({ | |
| plugins: [react()], |
|
|
||
| // Update the context with the image details | ||
| updateCanvasPhoto({ | ||
| id: Math.random().toString(36).substr(2, 9), |
Copilot
AI
Oct 27, 2025
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.
Using Math.random() for ID generation can produce collisions. Consider using crypto.randomUUID() for more reliable unique identifiers.
| id: Math.random().toString(36).substr(2, 9), | |
| id: crypto.randomUUID(), |
| "application/json", | ||
| ); | ||
| } catch (error) { | ||
| alert(`Error exporting print description ${error.message}`); |
Copilot
AI
Oct 27, 2025
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.
Accessing error.message without checking if error is an Error object will fail if the caught value is not an Error. Use type narrowing or convert to string: String(error) or error instanceof Error ? error.message : String(error).
| alert(`Error exporting print description ${error.message}`); | |
| alert(`Error exporting print description ${error instanceof Error ? error.message : String(error)}`); |
| }); | ||
| } | ||
| } catch (error) { | ||
| alert(`Error loading configuration: ${error.message}`); |
Copilot
AI
Oct 27, 2025
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.
Accessing error.message without checking if error is an Error object will fail if the caught value is not an Error. Use type narrowing or convert to string: String(error) or error instanceof Error ? error.message : String(error).
| alert(`Error loading configuration: ${error.message}`); | |
| alert(`Error loading configuration: ${error instanceof Error ? error.message : String(error)}`); |
| } from "@/helpers/inchesUtils"; | ||
| import { validatePrintDescriptionStructure } from "@/helpers/validatePrintDescriptionStructure"; | ||
|
|
||
| type PhotoEditorProps = {}; |
Copilot
AI
Oct 27, 2025
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.
The empty PhotoEditorProps type is unused since the component doesn't accept props. Remove this type definition.
| type PhotoEditorProps = {}; |
Key stuff about the code:
Design approach
The
requiermentsspecifically pointed out that this repo must be created as if it would be maintained for years to comeThat’s why I chose the Feature-Sliced Design (FSD) approach - it offers excellent maintainability by organizing the codebase around specific features. Each feature has its own folder containing all related logic and components, which helps avoid confusion about which components can be reused across the application and which are feature-specific.
Regarding the implementation of the app. The app clearly lacks some features. I suppose more photo editor features would be added further down the line. That is why I chose to create a Context and keeps state of the canvas there to make working with canvas easier when refactoring and adding more features. So the whole app lives under
printDescriptionContextTesting
Unit tests
In terms of good practices I added
vitestandreact testing libraryto perform unit tests.Unit or component tests are located near components they perform tests on since they are very dependant on implementation.
Integration or e2e tests,
I made a separate folder for integration tests but didn't do any, I think I should install playwright or maybe some phtoto comparison tests to perform canvas testing.
CI
I added
biomejsfor linting and formatting as it featureseasy-to-understandone file config. I also addedhuskyandpre-pushhook