ToCode Solution: תרגול מחזור חיים #36
Conversation
ynonp
left a comment
ynonp
left a comment
There was a problem hiding this comment.
I think you took a complex approach which was not necessary here. Please review my suggested solution:
https://forum.tocode.co.il/t/topic/421/2
| */ | ||
|
|
||
|
|
||
| import React from 'react'; |
There was a problem hiding this comment.
I think you took a complex approach which was not necessary here. Please review my suggested solution:
https://forum.tocode.co.il/t/topic/421/2
Mainly you don't need to manage keyboard focus by yourself. The browser already knows how to manage it. Moreover rewriting browser mechanisms is almost always a bad idea. Better to customise them to your needs.
| setFocus = (el,index) =>{ | ||
| if(el!=null){ | ||
| el.focus() | ||
| el.style.backgroundColor = "#ccc"; |
There was a problem hiding this comment.
Still some things worth mentioning: Don't set the background colour for a focused element in JS. Write the CSS rule div:focus { ... }
| fillText = (e,index) =>{ | ||
| let isFocused = (document.activeElement === e.target); | ||
| if(isFocused){ | ||
| if(document.activeElement.innerHTML != ''){ |
There was a problem hiding this comment.
Don't change the innerHTML AND the state. Choose one. The React way is to change state, which leads to render, which will change the DOM.
| activeEl.blur(); | ||
| let parentDiv = this.parent | ||
| if(activeEl.nextSibling!=null){ | ||
| this.setFocus(activeEl.nextSibling); |
There was a problem hiding this comment.
Notice setFocus will change the state, which will lead back to componentDidUpdate. This circle is an anti pattern. The hook componentDidUpdate should ONLY be used to make DOM modifications in response to state or props change that were not possible in render.
(For example you could use it to call focus on a DOM element)
| return (<div ref={parent => { this.parent = parent }}> | ||
| <SquareDiv focus={(e) =>this.setFocus(e.target)} typeKey={(e,index) => this.fillText(e,0)} text={this.state.texts[0]}/> | ||
| <SquareDiv focus={(e) =>this.setFocus(e.target)} typeKey={(e,index) => this.fillText(e,1)} text={this.state.texts[1]}/> | ||
| <SquareDiv focus={(e) =>this.setFocus(e.target)} typeKey={(e,index) => this.fillText(e,2)} text={this.state.texts[2]}/> |
There was a problem hiding this comment.
Better to call the attribute setFocus as it's going to be used as a handler to setFocus
| render(){ | ||
|
|
||
| return (<div ref={parent => { this.parent = parent }}> | ||
| <SquareDiv focus={(e) =>this.setFocus(e.target)} typeKey={(e,index) => this.fillText(e,0)} text={this.state.texts[0]}/> |
There was a problem hiding this comment.
Why not create them in a loop? So it'll be easier to change the count
|
|
||
| constructor(props) { | ||
| super(props); | ||
| this.state = {texts : []}; |
There was a problem hiding this comment.
I don't think you gained much by saving an array of objects here (vs. an array of strings). Code would have been simpler if texts was: ['x', 'y', '1', '2'] rather than your version: [{ txt: 'x'}, { txt: 'y' }, { txt: '1' }, { txt: '2' }]
2 participants
No description provided.