Skip to content
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

Open
wants to merge 7 commits into
base: dev
Choose a base branch
from
Open

Sketch Canvas Component #500

wants to merge 7 commits into from

Conversation

lsha0730
Copy link
Contributor

@lsha0730 lsha0730 commented Jul 26, 2023

Description

image

  • Made this input component
  • Debounced onChange exports as SVG string by default, possibly base64 if exportAsBase64 prop is true

@lsha0730 lsha0730 requested a review from meleongg July 26, 2023 22:31
@lsha0730 lsha0730 self-assigned this Jul 26, 2023
@github-actions
Copy link

github-actions bot commented Jul 26, 2023

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

Copy link
Member

@meleongg meleongg left a 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

src/components/ApplicationForm/Skills.js Show resolved Hide resolved
src/components/Input/SketchCanvas.js Show resolved Hide resolved
<QuestionHeading>question 21</QuestionHeading>
<SubHeading size="1.25em">Draw your favourite character!</SubHeading>
<SketchCanvas
width={600}
Copy link
Member

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

Copy link
Contributor

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 Show resolved Hide resolved
const [showPicker, setShowPicker] = useState(false)
const canvasRef = useRef()

const [debounceTimerId, setDebounceTimerId] = useState(null)
Copy link
Member

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?

Copy link
Member

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

Copy link
Contributor

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

const newTimerId = setTimeout(async () => {
if (!loadedRef.current) return

if (exportAsBase64) {
Copy link
Member

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@meleongg meleongg requested a review from DonaldKLee July 29, 2023 09:30
Copy link
Member

@DonaldKLee DonaldKLee left a 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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants