-
Notifications
You must be signed in to change notification settings - Fork 1
fix: full size canvas plus eraser #4
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: master
Are you sure you want to change the base?
Conversation
…other option after eraser
<FontAwesomeIcon icon={faDownload}/> | ||
</button> | ||
<input type="color" onChange={this.handleChangeComplete}/> | ||
<input type="color" ref={this.colorInput} onChange={this.handleChangeComplete}/> |
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 handler name can be more specific than handleChangeComplete
.
|
||
<button | ||
onClick={() => { | ||
this.colorInput.current.value="#ffffff"; |
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.
Don't change this directly. You should only change the state.
<FontAwesomeIcon icon={faDownload}/> | ||
</button> | ||
<input type="color" onChange={this.handleChangeComplete}/> | ||
<input type="color" ref={this.colorInput} onChange={this.handleChangeComplete}/> |
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.
Set value
to this.state.color
. That way, you won't have to explicitly set the value in the handler. You also won't need the ref
this way.
background: "#121212", | ||
color: "#000000", | ||
choice: "brush", | ||
canvas: {} as HTMLCanvasElement, |
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.
Canvas should not be a state, it should be a 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.
Basically, whatever data you need to render the component (color, choice, isDrawStarted, etc.) should either be a state or prop. UI elements like canvas, websocket connection, etc are important but they are not exactly data that you want to display. They should be stored inside refs. They are also mutable which might cause undefined behaviour with state: state and props should always be immutable.
Description
Created full size canvas with eraser option.
Type of change
New feature (non-breaking change which adds functionality)
Checklist:
My code follows the style guidelines of this project
I have performed a self-review of my own code
My changes generate no new warnings
Any dependent changes have been merged and published in downstream modules