-
Notifications
You must be signed in to change notification settings - Fork 1
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
Sketch Canvas Component #500
base: dev
Are you sure you want to change the base?
Conversation
Visit the preview URL for this PR (updated for commit 45dca66): https://nwplus-ubc--pr500-sketch-canvas-compon-cgz71128.web.app (expires Thu, 24 Aug 2023 09:25:41 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 8c7ea898e009e43455645bc310bcbccfc0f87e48 |
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.
AMAZING work Lincoln! SO SPEEDY! I did not know these libraries existed that could do this lol
Your overall approach LGTM, just a couple additional comments:
- mobile responsiveness -> see ThemeProvider.js for booleans you can use within styled-components
- I think loading from Firebase would be the next step given that you haven't touched the "value" prop passed from Skills.js -> SketchCanvas.js
<QuestionHeading>question 21</QuestionHeading> | ||
<SubHeading size="1.25em">Draw your favourite character!</SubHeading> | ||
<SketchCanvas | ||
width={600} |
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.
I don't think this is mobile responsive right now. Looking at the npm package, it looks like react-sketch-canvas supports web & mobile
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.
done (i think). I didn't touch height cuz I personally think it looks better with the same height across all devices, but lmk if you think otherwise!
src/components/Input/SketchCanvas.js
Outdated
const [showPicker, setShowPicker] = useState(false) | ||
const canvasRef = useRef() | ||
|
||
const [debounceTimerId, setDebounceTimerId] = useState(null) |
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.
Should we could extract the debouncer logic into its own hook to make things cleaner?
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.
see useDebounce in utilities.js
w/i this repo
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.
done (i think). lmk if you want me to change more things
src/components/Input/SketchCanvas.js
Outdated
const newTimerId = setTimeout(async () => { | ||
if (!loadedRef.current) return | ||
|
||
if (exportAsBase64) { |
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.
Not sure I follow why we want to save as SVG for the first save and then JPEG for the following saves.
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.
done
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.
Overall, great work on this feature! Amazed by how fast you completed it :)
Some notes
- Clicking on "next" after drawing and then "back" resets the drawing
- is it possible to render a image onto their canvas? if not, a warning message might be helpful to let users know that they need to restart if they leave the page. Or you can also just display their data64 image below the canvas so applicants know that they did draw something
- Can we display their drawing on the "Review your submission" page?
- Not mobile responsive as Melvin mentioned
- Are we making the drawing required? (leaving canvas empty allows us to move on)
Description