1
\$\begingroup\$

This is a continuation of previous questions. I just started using react and I've managed to fetch a xml file, convert it to json and then loop through the data. But I think there should be a better way to go through what I've done as I'm using a function I found on another SO answer and then used a map() within it.

Anyways here's the breakdown of my code.

Fetching the xml and converting to json:

componentDidMount() {
    axios.get('https://gist.githubusercontent.com/eMatsiyana/e315b60a2930bb79e869b37a6ddf8ef1/raw/10c057b39a4dccbe39d3151be78c686dcd1101aa/guestlist.xml')
    .then(res => {
      const xml = XMLMapping.load(res.data);
      var guests  = XMLMapping.tojson(xml);
      this.setState({guests: guests});
    });
  }

The results of the json in console:

Object{
  dataset{
   record{
    0{
      company{
        $t: "Skippad"
      }
      first_name{
        $t: "Keith"
      }
      last_name{
        $t: "Cook"
      }
    }
    1{
      company{
        $t: "Skippad"
      }
      first_name{
        $t: "Keith"
      }
      last_name{
        $t: "Cook"
      }
    }
   }
  }
}

I'm using this function to map the object and then using a map() within it:

function mapObject(object, callback) {
  return Object.keys(object).map(function (key) {
    return callback(key, object[key]);
  });
}

This is what the final mapping of the data looks like:

{mapObject(this.state.guests, (key, value) => {
  return <div key={key}>
    {value.record
    .filter(
      (item,index) => {
        return (
          item.first_name.$t.toLowerCase().includes(this.state.search.toLowerCase())
          //item.last_name.$t.toLowerCase().includes(this.state.search.toLowerCase())
          //item.company.$t.toLowerCase().includes(this.state.search.toLowerCase())
        )
      }
    )
    .map((item,index) => {
      return <div className="columns is-mobile" key={index}>
        <div className="column" key={index}>{item.first_name.$t} {item.last_name.$t} <span class="is-hidden-tablet"><br />{item.company.$t}</span></div> 
        <div className="column is-hidden-mobile" >{item.company.$t}</div>
        <div className="column is-hidden-mobile">
          <EmailFormdisplay guestid={index} />
        </div>
        <div className="column is-hidden-mobile">
          <PhoneFormdisplay guestid={index} />
          </div>
          <div className="column is-hidden-tablet is-one-third-mobile">
          <Dropdown>
            <DropdownTrigger></DropdownTrigger>
              <DropdownContent>
                <div className="columns">
                  <div className="column">
                    <EmailFormdisplay guestid={index} />
                  </div>
                  <div className="column">
                    <PhoneFormdisplay guestid={index} />
                  </div>
                </div>
              </DropdownContent>
          </Dropdown>
          </div>
      </div>;
    })}
    </div>
  })}

Is there a better way of doing this without using mapObject() and a map() within it?

Any kind of feedback or advice would be greatly appreciated!

\$\endgroup\$
3
\$\begingroup\$

A few things:

  1. I'm not sure why you need to map over the top level object? Are there top-level keys other than dataset that you're interested in? From your example, it looks like you only need the array of records (guests), in which case you don't need mapObject.
  2. I usually try to keep JSX expressions as slim as possible. Your JSX is quite long, and I can see 3 opportunities to shorten/simplify it:
    1. Push more of the transformation into componentDidMount, making the data easier to work with during render
    2. If some transformation has to be done during render (e.g. the search filtering), then consider assigning it to an intermediate local variable rather than doing it inline in JSX
    3. Extract smaller components for logical sections
  3. I'm guessing that those commented out lines were meant to be || (or) clauses?
  4. Try naming variables according to the domain model. I.e., guest rather than item.
  5. There's a bit of duplication that can be reduced, like lower-casing the search string.

Here's a complete example (haven't tested it!):

class Guests extends React.Component {
  constructor(props) {
    super(props);
    this.state = { guests: [] };
  }

  componentDidMount() {
    axios.get('https://gist.githubusercontent.com/eMatsiyana/e315b60a2930bb79e869b37a6ddf8ef1/raw/10c057b39a4dccbe39d3151be78c686dcd1101aa/guestlist.xml')
      .then(res => {
        const xml = XMLMapping.load(res.data);
        const json = XMLMapping.tojson(xml);
        this.setState({ guests: json.dataset.record });
      });
  }

  render() {
    const search = this.state.search.toLowerCase();
    const guests = this.state.guests.filter(guest => (
      guest.first_name.$t.toLowerCase().includes(search) ||
        guest.last_name.$t.toLowerCase().includes(search) ||
        guest.company.$t.toLowerCase().includes(search)
    ));

    return (
      <div>
        {guests.map((guest, index) => (
          <div className="columns is-mobile" key={index}>
            <Guest first_name={guest.first_name.$t} last_name={guest.last_name.$t} company={guest.company.$t} />
            <div className="column is-hidden-mobile" >{guest.company.$t}</div>
            <div className="column is-hidden-mobile">
              <EmailFormdisplay guestid={index} />
            </div>
            <div className="column is-hidden-mobile">
              <PhoneFormdisplay guestid={index} />
            </div>
            <GuestDropdown index={index} />
          </div>
        ))}
      </div>
    );
  }
}

const Guest = ({ first_name, last_name, company }) => (
  <div className="column">
    {first_name} {last_name}
    <span class="is-hidden-tablet"><br />{company}</span>
  </div> 
);

const GuestDropdown = ({ index }) => (
  <div className="column is-hidden-tablet is-one-third-mobile">
    <Dropdown>
      <DropdownTrigger />
      <DropdownContent>
        <div className="columns">
          <div className="column">
            <EmailFormdisplay guestid={index} />
          </div>
          <div className="column">
            <PhoneFormdisplay guestid={index} />
          </div>
        </div>
      </DropdownContent>
    </Dropdown>
  </div>
);

Hope that helps :)

|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ Thank you for the detailed answer. I've added your script, the only that that was giving an issue was the state.search but I've included it in the props and it's working great :) \$\endgroup\$ – msbodetti Oct 10 '17 at 13:25
  • \$\begingroup\$ Glad it helped! I fear I may have gone too far in writing the whole completed code out for you :P Just make sure you understand the changes I made and why, and you'll be able to apply the principles on your own to your next React component :) \$\endgroup\$ – Cam Jackson Oct 10 '17 at 13:32
  • \$\begingroup\$ Thank you so much. Yes I need to start use your principles when writing out my own react app. Just another question, if I want to write a function, I should write it out as a const and then use it in the return? I'm guessing that's the normal for react coding or jsx? \$\endgroup\$ – msbodetti Oct 10 '17 at 13:38
  • \$\begingroup\$ It's a little difficult to answer your question without more context, but in short: yes, it's perfectly normal to extract code from a component as a function, and then call it from inside the JSX expression. \$\endgroup\$ – Cam Jackson Oct 16 '17 at 11:41

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.