-Created UI functionality for changing room name#59
-Created UI functionality for changing room name#59MuhammadSaadQadeer wants to merge 2 commits intolukejacksonn:masterfrom MuhammadSaadQadeer:change-room-name
Conversation
-Removed unnecessary console.log
lukejacksonn
left a comment
lukejacksonn
left a comment
There was a problem hiding this comment.
Hey @MuhammadSaadQadeer I just pulled down this PR to test out the feature. It is looking good so far.. it worked! I am going to go through the code and make some comments where I think things could be improved.
| </svg> | ||
| <svg id="done" viewBox="0 0 512 512"> | ||
| <path d="M0 0h24v24H0z" fill="none"/> | ||
| <path d="M256 8C119.033 8 8 119.033 8 256s111.033 248 248 248 248-111.033 248-248S392.967 8 256 8zm0 48c110.532 0 200 89.451 200 200 0 110.532-89.451 200-200 200-110.532 0-200-89.451-200-200 0-110.532 89.451-200 200-200m140.204 130.267l-22.536-22.718c-4.667-4.705-12.265-4.736-16.97-.068L215.346 303.697l-59.792-60.277c-4.667-4.705-12.265-4.736-16.97-.069l-22.719 22.536c-4.705 4.667-4.736 12.265-.068 16.971l90.781 91.516c4.667 4.705 12.265 4.736 16.97.068l172.589-171.204c4.704-4.668 4.734-12.266.067-16.971z"/> |
There was a problem hiding this comment.
Indentation looks off here, have you heard of eslint? It will be super useful for things like this.
There was a problem hiding this comment.
@lukejacksonn should I change this, using eslint indentation?
There was a problem hiding this comment.
There is no .eslint config in this repo but I'd give it a run and see what it spits out!
There was a problem hiding this comment.
Okay , I'll leave that to you. Btw is there any other medium apart from git to communicate , like pm etc. If there is can you tell me ? I'd like to communicate with you there.
|
|
||
| { | ||
| inputIsOpen && | ||
| <h1>{[ |
There was a problem hiding this comment.
You could un-nest this a bit.. perhaps what we should be aiming for is something like:
<form className={style.horizontal}>
<input />
<button />
</form>
There was a problem hiding this comment.
@lukejacksonn are you talking about changing the indentation or logic here ?
There was a problem hiding this comment.
Just not wrapping things so much.. like, having a form in an h1 feels a bit wrong.
There was a problem hiding this comment.
I had wrapped the form in h1 because h1 had styles properties that were used by the heading before editable field is set true. So what I can do now is that to make another style object with same properties and apply it to form. But it will be like duplicating things. Apart from that I don't think that we need a form here either. If I remove it it'll work fine. So, should I remove it ?
| { | ||
| inputIsOpen && | ||
| <h1>{[ | ||
| <form > |
There was a problem hiding this comment.
Good practice when using forms is to add an onSubmit prop that, at least does e.preventDefault otherwise if someone hits return inside the form then the page gets reloaded.
There was a problem hiding this comment.
You could also call the updateRoom function from within onSubmit and then make your SVG a button type='submit'.. then when someone clicks on it or presses enter, then it will do the right thing 👌
There was a problem hiding this comment.
What exactly are you implying , I am missing your point here, do you want me to remove the svg and add a button & I haven't used onSubmit prop, I did use it in redux forms but that was different scenario. Can you elaborate a bit more ?
There was a problem hiding this comment.
Here is a form that handles both the user hitting enter and them clicking the submit button. This way the input it not controlled but left to do its thing until the user submits the form. Which requires less wiring (no changeInputTitle action or roomTitle in state).
<form onSubmit={e => {
e.preventDefault()
updateRoom(room.id, e.target.elements[0].value)
}}>
<input />
<button type='submit'><svg /></button>
</form>
Does that make sense? What do you think?
3 participants
No description provided.