Norfeldt Norfeldt - 1 month ago 7
React JSX Question

React: Is this a good way to use props and states

I finally got a responsive setup that actually works. But I would very much like to get some feedback on whether this a good setup and data flow.

I made the following:

App.js



import React, { Component } from 'react';
import './App.css';
import AB_eval from './components/AB_eval';

class App extends Component {
constructor() {
super();
this.updateDB = this.updateDB.bind(this);
// getInitialState
this.state = {
DB: []
};
}

updateDB(k, bar) {
const DB = {...this.state.DB}
DB[k] = bar;
this.setState({ DB });
}

render() {
return (
<div className="App">

<AB_eval A={2} B={3} updateDB={this.updateDB} />

</div>
);
}
}

export default App;


AB_eval.js



import React, { Component } from 'react';
import Slider from 'react-rangeslider';

class AB_eval extends Component {
constructor(props, context){
super(props, context);

this.updateDB = this.updateDB.bind(this);
this.refreshAB = this.refreshAB.bind(this);

let {A, B} = this.props;
let AB = A * B;
this.state = {
A: A,
B: B,
AB: AB
}
}

componentDidMount () {
this.updateDB();
}

refreshAB () {
const AB = this.state.A * this.state.B;
this.setState({
AB: AB
});
}

updateDB () {
const bar = {
A: this.state.A,
B: this.state.B,
AB: this.state.AB
}
this.props.updateDB(0, bar) // updating the App's state
}

render() {
let {A, B, AB} = this.state;
return (
<div>
<h1>AB_eval: {AB}</h1>


<h2>A</h2>
<p>A: {A} B: {B} AB:{AB}</p>
<Slider min={1} max={4} step={1}
value={A}
onChange={ (value)=> {
this.setState({
A: value
});
this.refreshAB()
this.updateDB() }
}
/>

<h2>B</h2>
<p>A: {A} B: {B} AB:{AB}</p>
<Slider min={1} max={4} step={1}
value={B}
onChange={ (value)=> {
this.setState({
B: value
});
this.refreshAB()
this.updateDB() }
}
/>
</div>
)
}
}

AB_eval.propTypes = {
A: React.PropTypes.number.isRequired,
B: React.PropTypes.number.isRequired,
updateDB: React.PropTypes.func.isRequired
};

export default AB_eval;


Next up is to add a C slider - so it will become an ABC_eval. But first I need to know:


  • Are the data flow between App and the AB_eval a good approach?

  • Would it make sense to break things down to more components?






Update



I realised that using a
setState
inside the
AB_eval
component wasnt such a good idea after all when looping through the state.

I updated the code to the following, but it isn't working.. think Im missing a couple of things..

dummy-data.js



module.exports = {
one: {A: 1,
B: 2,
AB: 2
},
two: { A: 3,
B: 4,
AB: 12
}
};


App.js



import React, { Component } from 'react';
import './App.css';
import AB_eval from './components/AB_eval';
import dummyData from './dummy-data';

class App extends Component {
constructor() {
super();
this.updateDB = this.updateDB.bind(this);

// getInitialState
this.state = {
DB: dummyData
};
}

updateDB(key, bar) {
const DB = {...this.state.DB}
DB[key] = bar;
this.setState({ DB });
}

render() {
return (
<div className="App">
<ul className="list-of-ABs">
{ Object
.keys(this.state.DB)
.map(key =>
<AB_eval
key = {key}
ID = {key}
Data = {this.state.DB[key]}
updateDB={this.updateDB} />
)
}
</ul>
</div>
);
}
}

export default App;


AB_eval.js



import React, { Component } from 'react';
import Slider from 'react-rangeslider';

class AB_eval extends Component {
constructor(props, context){
super(props, context);

this.updateDB = this.updateDB.bind(this);

// Trying to make these variable instance variables
const {ID, Data} = this.props;

}

updateDB () {
this.props.updateDB(this.ID, this.Data) // updating the App's state
}

render() {
console.log(this.Data);
let {A, B, AB} = this.Data;
return (
<div>
<h1>AB_eval: {this.ID}</h1>

<h2>A</h2>
<p>A: {A} B: {B} AB:{AB}</p>
<Slider min={1} max={4} step={1}
value={A}
onChange={ (value)=> {
this.A = value;
this.AB = this.A * this.B;
this.updateDB();
}
}
/>

<h2>B</h2>
<p>A: {A} B: {B} AB:{AB}</p>
<Slider min={1} max={4} step={1}
value={B}
onChange={ (value)=> {
this.B = value;
this.AB = this.A * this.B;
this.updateDB();
}
}
/>
</div>
)
}
}

AB_eval.propTypes = {
ID: React.PropTypes.string.isRequired,
Data: React.PropTypes.object.isRequired,
updateDB: React.PropTypes.func.isRequired
};

export default AB_eval;


(not sure if I use the
const
and
let
at the right places)




SOLUTION



With help from @Pineda and some trying I found the following solution to work as I wish:


scr/App.js


import React, { Component } from 'react';
import './App.css';
import ABEval from './components/ABEval';
import dummyData from './dummy-data';

class App extends Component {
constructor() {
super();
this.updateDB = this.updateDB.bind(this);

// getInitialState
this.state = {
DB: dummyData
};
}

updateDB(key, bar) {
const DB = {...this.state.DB}
DB[key] = bar;
this.setState({ DB });
}

render() {
return (
<div className="App">
<ul className="list-of-ABs">
{ Object
.keys(this.state.DB)
.map(key =>
<ABEval
key={key}
ID={key}
Data={this.state.DB[key]}
updateDB={this.updateDB} />
)
}
</ul>
</div>
);
}
}

export default App;



scr/dummy-data.js


module.exports = {
one: {A: 1,
B: 2,
AB: 2
},
two: { A: 3,
B: 4,
AB: 12
}
};



scr/components/ABEval.js


import React, { Component } from 'react';
import XSlider from './XSlider';

class AB_eval extends Component {
constructor(props, context){
super(props, context);
console.log(`${this.props.ID}: Constructed`);
}

componentDidMount(){
console.log(`${this.props.ID}: Mounted`);
}

render() {
console.log(`${this.props.ID}: rendered`)
const { Data, ID } = this.props;
const { A, B, AB } = Data;
return (
<div>
<h1>ABEval: {ID}</h1>
<p>A: {A} B: {B} AB:{AB}</p>

<XSlider
title={'A'}
value={A}
valueHandler={
(val)=> this.props.updateDB(ID, {A: val, B: B, AB: val*B} )}
/>

<XSlider
title={'B'}
value={B}
valueHandler={
(val)=> this.props.updateDB(ID, {B: val, A: A, AB: val*A} )}
/>
</div>
)
}
}

AB_eval.propTypes = {
ID: React.PropTypes.string.isRequired,
Data: React.PropTypes.object.isRequired,
updateDB: React.PropTypes.func.isRequired
};

export default AB_eval;



scr/components/XSlider.js


import React from 'react';
import Slider from 'react-rangeslider';

export default ({title, value, valueHandler}) => {
return (
<div>
<h2>{title}</h2>
<Slider min={1} max={4} step={1} value={value} onChange={valueHandler} />
</div>
);
}

Answer

With the information you've provided it's difficult to make many re-factoring recommendations but the data flow looks ok.

Issues within your latest AB_eval.js:

const {ID, Data} = this.props;  // block scoped, DOES NOT create class members
                                // referencable by this.ID or this.DATA
                                // outside the scope of its containing block

const is block scoped so your destructuring of props within the constructor will only create a useable value of ID and Data within the constructor block.

This breaks later references of this.ID and this.Data (in the updateDB() method). References to this.A, this.AB, this.B and this.updateDB in the render() method will also be broken. To fix this, I suggest destructuring the props within the block scope of your render and onChange handler(s).

this.AB = this.A * this.B;  // state should be treated as immutable
                            // and since props are propogated by changes in state
                            // mutating them is ill-advised

Attempting to set values on this.props.Data directly within the onChange methods which is effectively mutating what should be treated as immutable.

There are a couple ways you can ensure no mutations using ES6/ES7 notation, one is using Object.assign and the other is using the Spread syntax notation.

Solution

AB_eval.js:

import React, { Component } from 'react';
import Slider from 'react-rangeslider';

class AB_eval extends Component {
  constructor(props, context){
    super(props, context);

    this.onChangeA = this.onChangeA.bind(this);
    this.onChangeB = this.onChangeB.bind(this);
  }

  onChangeA (value) {
    const {ID, Data, updateDB} = this.props;
    updateDB(ID, {
      ...Data,
      A: value,
      AB: value * Data.B
    });
  }

  onChangeB (value) {
    const {ID, Data, updateDB} = this.props;
    updateDB(ID, {
      ...Data,
      B: value,
      AB: value * Data.B
    });
  }

  render() {
    const {A, B, AB} = this.props.Data;
    return (
      <div>
        <h1>AB_eval: {this.props.ID}</h1>

        <h2>A</h2>
        <p>A: {A} B: {B} AB:{AB}</p>
        <Slider min={1} max={4} step={1}
          value={A}
          onChange={this.onChangeA}
        />

        <h2>B</h2>
        <p>A: {A} B: {B} AB:{AB}</p>
        <Slider min={1} max={4} step={1}
          value={B}
          onChange={this.onChangeB}
          }
        />
      </div>
    )
  }
}

AB_eval.propTypes = {
  ID: React.PropTypes.string.isRequired,
  Data: React.PropTypes.object.isRequired,
  updateDB: React.PropTypes.func.isRequired
};

export default AB_eval;
  • The single updateDB method was replaced with two onChange handlers (onChangeA and onChangeB respectively) to handle the slider cases for A and B.
  • the props were destructured accordingly within these handlers as well as within the render function, notice the use of the spread syntax in order to create the updated an object that doesn't mutate the existing this.Data.props
  • The onChange handlers no longer use the fat arrow notation you had before so it is necessary to bind both the handlers to this.

UPDATE Regarding refactoring:
You could extract the Slider code out into a functional component (I've added a folder structure just as a suggestion):

./components/AB_eval_slider.js

import React from 'react';

export default ({key, db, handler}) => {
  return (
    <div>
      <h2>{key}</h2>
      <p>A: {db.A} B: {db.B} AB:{db.AB}</p>
      <Slider min={1} max={4} step={1} value={key} onChange={handler} />
    </div>
  );
}

So you'd also have to edit my suggested AB_eval.js to include:
import AB_eval_Slider from './components/AB_eval_slider';

and the render method would now be:

render() {
  return (
    const { Data } = this.props;
    <div>
      <h1>AB_eval: {this.ID}</h1>
      <AB_eval_Slider key={'A'} db={Data}, handler={this.onChangeA} />
      <AB_eval_Slider key={'B'} db={Data}, handler={this.onChangeB} />
    </div>
  );
}