Skip to content
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
90 changes: 80 additions & 10 deletions 33-lifecycle-lab_v16/src/03-custominput.jsx
Original file line number Diff line number Diff line change
@@ -1,10 +1,80 @@
/**
* Implment an input component that shows 4
* input boxes writing user input in the currently
* focused panel
*
* See lab description on the webpage for a live example
* how this should work.
*/


import React from 'react';
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

import PropTypes from 'prop-types';

export default class SquareFocus extends React.Component{

constructor(props) {
super(props);
this.state = {texts : []};
Copy link
Collaborator

Choose a reason for hiding this comment

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

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' }]

}


setFocus = (el,index) =>{
if(el!=null){
el.focus()
el.style.backgroundColor = "#ccc";
Copy link
Collaborator

Choose a reason for hiding this comment

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

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 != ''){
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

let updatedArr = this.state.texts;
updatedArr[index] = {txt : String.fromCharCode(e.keyCode)};
this.setState({texts:updatedArr});
}else{
this.setState({texts:[...this.state.texts , {txt : String.fromCharCode(e.keyCode)} ]});
}
}
}

componentDidUpdate(){
let activeEl = document.activeElement;
activeEl.style.backgroundColor = "white";
activeEl.blur();
let parentDiv = this.parent
if(activeEl.nextSibling!=null){
this.setFocus(activeEl.nextSibling);
Copy link
Collaborator

Choose a reason for hiding this comment

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

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)

}else{
parentDiv.children[0].focus()
parentDiv.children[0].style.backgroundColor = "#ccc";
}
}

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]}/>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not create them in a loop? So it'll be easier to change the count

<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]}/>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Better to call the attribute setFocus as it's going to be used as a handler to setFocus

<SquareDiv focus={(e) =>this.setFocus(e.target)} typeKey={(e,index) => this.fillText(e,3)} text={this.state.texts[3]}/>
</div>);
}

}

function SquareDiv(props){
const char = {
display: 'inline-block',
verticalAlign: 'top',
height: '100px',
width: '100px',
border: '1px solid',
margin: '0 5px',
textAlign: 'center',
lineHeight: '100px',
fontSize: '32px'
}


return <div tabIndex='1' style={char} onFocus={props.focus} onKeyDown={props.typeKey}>
{props.text.txt}
</div>
}


SquareDiv.propTypes = { focus: React.PropTypes.func,
typeKey: React.PropTypes.func,
text: React.PropTypes.object};
SquareDiv.defaultProps = {text: {txt:""} };