2

I have the following code and I'm trying to simplify it using a map function, perhaps on the array: const columns = ['Title', 'Author', 'Rating']

export const BookshelfListRow = (props) => {

  return (
    <tr className="table-row" >


      <td>
        <input onChange={(e) => { props.Update(e.target.value) }} placeholder={props.book.Title} />
      </td>

      <td>
        <input onChange={(e) => { props.Update(props.book.Title, e.target.value) }} placeholder={props.book.Author} />
      </td>
      
      <td>
        <input onChange={(e) => { props.Update(props.book.Title, props.book.Author, e.target.value) }} placeholder={props.book.Rating} />
      </td>


    </tr>
)}

Please note this is simplified - in my actual code I have 30 columns (meaning 30 separate inputs instead of 3) hence why I'm looking for a way to simplify it as it is currently really long - so essentially what is happening above is the placeholder is iterating through the array [Title,Author,Rating], and simultaneously on each new line we are adding an item from the array (in the form of props.book[item]) to the props.Update function. Any ideas how I could use a map function to carry this out?

ibrahim mahrir
  • 28,583
  • 5
  • 34
  • 61
SJ9991
  • 23
  • 5
  • Your first `td`'s `onChange` is different than the rest, is that intentional? – ibrahim mahrir Jul 20 '20 at 00:07
  • Can you add the function that is passed as `props.Update` to your question? – Chris G Jul 20 '20 at 00:12
  • Yes it is intentional, so with each input you add a new argument which comes from the array to the props.Update() function – SJ9991 Jul 20 '20 at 00:13
  • ```const Update = (Title,Author,Rating) => { axios.put('http://localhost:4001/books/update', { Title:Title, Author: Author, Rating:Rating }) }``` – SJ9991 Jul 20 '20 at 00:18

4 Answers4

1

You can use map to simplify it. The tricky bit will be the calling of Update with different number of parameters, but that too can be achieved using another map.

const columns = ['Title', 'Author', 'Rating'];

export const BookshelfListRow = (props) => {
  return (
    <tr className="table-row">
    {
      columns.map((column, i) => (
        <td>
          <input onChange={ e =>
                   props.Update(...[                                            // the parameters to Update consist of
                     ...columns.slice(0, i).map(column => props.book[column]),  // the column values from the start until the current column, map is used here to get the values for those columns
                     e.target.value                                             // and the input value
                   ])
                 }
                 placeholder={ props.book[column] } />
        </td>
      ))
    }
    </tr>
  )
}

Another approach:

The Update function is a mess. It can be a lot simpler if it just takes the column that was changed and the value as there is no need for it to send all those props back to the server if only one was changed, like so (this uses computed property names):

const Update = (column, value) =>                                         // takes the column that was changed and the value
  axios.put('http://localhost:4001/books/update', { [column]: value });   // update only that column

Then the rendering will be much simpler also, like so:

const columns = ['Title', 'Author', 'Rating'];

export const BookshelfListRow = (props) => {
  return (
    <tr className="table-row">
    {
      columns.map((column, i) => (
        <td>
          <input onChange={ e => props.Update(column, e.target.value) } placeholder={ props.book[column] } />
        </td>
      ))
    }
    </tr>
  )
}
ibrahim mahrir
  • 28,583
  • 5
  • 34
  • 61
  • Yes I think cleaning up the update function is the way to go - however I wouldn't be able to use ```const columns = ['Title', 'Author', 'Rating']``` as I'd need to map over ```Object.keys(props.book)``` instead right? – SJ9991 Jul 20 '20 at 10:03
  • Your first solution also works exactly as expected - thank you however I will try to implement your alternative approach as that does seem cleaner – SJ9991 Jul 20 '20 at 10:23
  • @SJ9991 You can use `Object.keys(props.book)` but [**there is no guarantee that the keys will be in the order you want them to be**](https://stackoverflow.com/q/5525795/9867451), why can't you use the variable `columns`? You can declare it inside the function `BookshelfListRow` right before the return if you don't want it to be global. Also, if you use the second approach then `Object.keys` will be ok to use because the order of the keys in the second approach is not very important as `Update` only takes one column at a time. – ibrahim mahrir Jul 20 '20 at 10:45
  • I managed to change the Update function and now works perfectly using ```Object.keys(props.book)``` Thanks again! – SJ9991 Jul 20 '20 at 11:38
0

const columns = ['Title', 'Author', 'Rating']

const update = (val, column) => {
  console.log(`${column}: ${val}`)
}

const BookshelfListRow = () => (<table><tbody><tr className="table-row">{
      columns.map((column, i) => {
        return (<td key={i}><input type="text" onChange = {e => update(e.target.value, column)} placeholder={column} /></td >)
      })
    }</tr></tbody></table>
  )


ReactDOM.render(
  <BookshelfListRow />,
  document.getElementById("root")
);
<div id="root"></div>
<script src="https://cdnjs.cloudflare.com/ajax/libs/react/16.6.3/umd/react.production.min.js"></script>
<script src="https://cdnjs.cloudflare.com/ajax/libs/react-dom/16.6.3/umd/react-dom.production.min.js"></script>
ksav
  • 13,381
  • 5
  • 27
  • 51
0

If you're using the keys of props.book, you can try something like this:

import React from "react";

const BookshelfListRow = props => {
  const args = [];
  return (
    <tr className="table-row">
      {Object.keys(props.book).map((key, idx) => {
        if(idx > 0) {
          args.unshift(key);
        }
        const argsCopy = [...args];
        return (
          <td>
            <input
              onChange={e => {
                props.Update(...argsCopy, e.target.value);
              }}
              placeholder={props.book[key]}
            />
          </td>
        );
      })}
    </tr>
  );
};

export default BookshelfListRow;

Otherwise, you can use an array like the one you suggested (const columns = ['Title', 'Author', 'Rating']) and take each value and add it to a copy with each map loop.

Micah Wierenga
  • 103
  • 1
  • 2
  • 9
  • I think the args.unshift should be args.push and this is very close - the main problem is e.g for the first input tag, the ```props.Update(...argsCopy, e.target.value)``` would return ```props.Update('Title', e.target.value)``` rather than ```props.Update(props.book.Title, e.target.value)``` – SJ9991 Jul 20 '20 at 09:51
0

Mapping is a very powerful tool in React. It is most useful when you are try to DRY out some repeated code. In your case you are trying to DRY out your td's by mapping over the array columns.

Your columns array will need a little more info to make mapping useful. For instance,

const columns = ['Title', 'Author', 'Rating']

columns.map(column => console.log(column)) // Title, Author, Rating

That's not very helpful for your td because it needs the onChange, and placeholder and both require more information than just the strings 'Title', 'Author', and 'Rating'.

From what I can tell, your book prop is an object that looks something like this:

book: { 
  Title: 'some string', 
  Author: 'some other string',
  Rating: 'some number maybe'
}

You can map over that by using Object.keys. But again, that only helps with the placeholder not the onChange.

The data that you have and the data you are trying to use for your inputs do not seem to have a common enough pattern to utilize map here.

Possible Solution

Modify your update function to not require so many parameters to keep the input field generic as possible that way you can map over your columns.

export const BookshelfListRow = (props) => {
  // if you are using React without hooks, just replace this with
  // normal state
  const [state, setState] = useState({ 
    title: '',
    author: '',
    rating: ''
  })
  const update = (e) => { 
    const input = e.currentTarget.value; 
    const attribute = e.currentTarget.name;

    setState({...state, [attribute]: input})
  }

  const apiRequest = (e) => { 
    e.preventDefault();

    // your state is now your request body
    request(state)
  } 
  const columns = ['Title', 'Author', 'Rating']
  return (
    <tr className="table-row" >
      {columns.map(column => (
        <td key={column}>
          <input name={column.toLowerCase()} onChange={update} placeholder={column} />
        </td>
      ))}
    </tr>
)}
Bens Steves
  • 1,513
  • 1
  • 11
  • 19
  • Your update and apiRequest functions can be placed in your parent and access via props. I dumped it all into the BookshelfListRow component to keep it concise. – Bens Steves Jul 20 '20 at 00:33