8
\$\begingroup\$

I have written a small app that fetches news items from an endpoint and displays them in a grid.

I used React to create components and use them throughout the app. This is the first thing I have built using ES6 - mainly for syntactic sugar.

I tried to match the BBC News page style, so it should look very similar. It's responsive and should look good on any size screen.

Overall, I'd mainly like feedback for my React and ES6, comments on best practices or what I could do better.

Here is my React code:

const Header = () => {
    return (
        <div className="white-header">
            {/* Bootstrap nav */}
            <nav className="navbar">
                <div className="container-fluid">
                    <div className="navbar-header">
                        <button type="button" className="navbar-toggle collapsed" data-toggle="collapse"
                                data-target="#bs-example-navbar-collapse-1" aria-expanded="false">
                            <span className="sr-only">Toggle navigation</span>
                            <i className="fa fa-bars icon-bar"></i>
                        </button>
                        <div className="navbar-brand" href="#">
                            <span className="fccLogo">F</span>
                            <span className="fccLogo">C</span>
                            <span className="fccLogo">C</span>
                        </div>
                    </div>

                    <div className="collapse navbar-collapse" id="bs-example-navbar-collapse-1">
                        <ul className="nav navbar-nav">
                            <li><a href="http://www.freecodecamp.com/">freecodecamp</a></li>
                            <li><a href="http://www.bbc.co.uk/news">bbc news</a></li>
                            <li><a href="http://github.com/alanbuchanan">github</a></li>
                        </ul>
                    </div>
                </div>
            </nav>

            <div className="red-header">
                <h1>NEWS</h1>
            </div>

            <div className="darkred-header">
                <h1></h1>
            </div>
        </div>
    );
};

const BigStory = (props) => {

    const {newsItems} = props;
    let {headline} = newsItems;

    headline = splitHeadlineAtUnwantedChar(headline);

    return (
        <div className="big-story col-xs-12">
            <div className="col-sm-5">
                <h1><HeadlineLink headline={headline} link={newsItems.link}/></h1>

                <p>{newsItems.metaDescription}</p>
                <TimeAndLink time={newsItems.timePosted} author={newsItems.author.username}/>
            </div>
            <div className="col-sm-7">
                <img className="img-responsive" src={newsItems.image} alt=""/>
            </div>
        </div>
    );
};

const MediumStory = (props) => {

    const {newsItems} = props;

    newsItems.headline = splitHeadlineAtUnwantedChar(newsItems.headline);

    return (
        <div className="medium-story col-sm-4 col-xs-6">
            <img className="img-responsive" src={newsItems.image} alt=""/>

            <h4><HeadlineLink headline={newsItems.headline} link={newsItems.link}/></h4>

            <p>{newsItems.metaDescription}</p>
            <TimeAndLink time={newsItems.timePosted} author={newsItems.author.username}/>
        </div>
    );
};

const SmallStory = (props) => {

    const {newsItems} = props;
    newsItems.headline = splitHeadlineAtUnwantedChar(newsItems.headline);

    return (
        <div className="small-story">
            <h4><HeadlineLink headline={newsItems.headline} link={newsItems.link}/></h4>
            <TimeAndLink time={newsItems.timePosted} author={newsItems.author.username}/>
        </div>
    );
};

const DatedListNoPics = (props) => {

    const {items} = props;

    const list = items.map((e, i) => {
        return (
            <li className="col-sm-6" key={i}>
                <h5><HeadlineLink headline={splitHeadlineAtUnwantedChar(e.headline)} link={e.link}/></h5>
                <TimeAndLink time={e.timePosted} author={e.author.username}/>
            </li>
        );
    });

    return (
        <ul className="dated-list-no-pics">
            {list}
        </ul>
    );
};

const DatedListWithPics = (props) => {

    let {items} = props;

    items = filterForImages(items);

    const list = items.map((e, i) => {
        return (
            <div className="col-lg-12 col-md-6 col-sm-6" key={i}>
                <div className="col-md-6 col-sm-6 col-xs-6">
                    <img className="img-responsive" src={e.image} alt=""/>
                </div>
                <div className="col-md-6 col-sm-6 col-xs-6">
                    <h4><HeadlineLink headline={splitHeadlineAtUnwantedChar(e.headline)} link={e.link}/></h4>

                    <TimeAndLink time={e.timePosted} author={e.author.username}/>
                </div>
            </div>
        );
    });

    return (
        <div className="dated-list-with-pics">
            {list}
        </div>
    );
};

// Helpers and mini components

const splitHeadlineAtUnwantedChar = (str) => str.indexOf("—") !== -1 ? str.split("—")[0] : str;
const filterForImages = (arr) => arr.filter(e => e.image !== "");
const Loading = () => <div></div>;

const HeadlineLink = (props) => {
    return (
        <div className="headline-link">
            <a href={props.link}>{props.headline}</a>
        </div>
    );
};

const Main = React.createClass({

    getInitialState () {
        return {
            newsItems: []
        };
    },

    componentDidMount () {
        this.getNewsItems();
    },

    getNewsItems () {
        $.getJSON("http://www.freecodecamp.com/news/hot", (data) => {
            this.setState({newsItems: data});
        });
    },

    render () {
        const {newsItems} = this.state;
        const loading = newsItems.length === 0;
        let listNoPics = [];
        let listWithPics = [];
        const storiesToShow = 25;

        // This is done in the render to avoid further ternary operators due to loading, as below
        // List 1 (no pics):
        for (let i = 6; i <= 11; i++) {
            listNoPics.push(newsItems[i]);
        }

        // List 2 (with pics):
        for (let i = 12; i <= storiesToShow; i++) {
            listWithPics.push(newsItems[i]);
        }

        return (
            <div className="container">
                <Header />

                <div className="main-content col-sm-12">
                    <div className="left-sided-lg-top-otherwise col-lg-8 col-md-12 col-sm-12 col-xs-12">
                        {loading
                            ? <Loading />
                            : <BigStory newsItems={newsItems[0]}/>
                        }
                        {loading
                            ? <Loading />
                            : <MediumStory newsItems={newsItems[1]}/>
                        }
                        {loading
                            ? <Loading />
                            : <MediumStory newsItems={newsItems[2]}/>
                        }
                        <div className="col-sm-4 col-xs-12">
                            {loading
                                ? <Loading />
                                : <SmallStory newsItems={newsItems[3]}/>
                            }
                            {loading
                                ? <Loading />
                                : <SmallStory newsItems={newsItems[4]}/>
                            }
                            {loading
                                ? <Loading />
                                : <SmallStory newsItems={newsItems[5]}/>
                            }
                        </div>
                        {loading
                            ? <Loading />
                            : <DatedListNoPics items={listNoPics}/>
                        }
                    </div>
                    <div className="right-sided-lg-bottom-otherwise col-lg-4 col-md-12 col-sm-12 col-xs-12">
                        {loading
                            ? <Loading />
                            : <DatedListWithPics items={listWithPics}/>
                        }
                    </div>
                </div>
            </div>
        );
    }
});

// Place right at the end because it messes up the colouring. 
const TimeAndLink = (props) => {
    return (
        <p className="time-and-link">
            <span id="timeago"><i className="fa fa-clock-o"></i> {$.timeago(props.time).replace(/(about)/gi, "")}</span> | <a href={`http://www.freecodecamp.com/${props.author}`}>{props.author}</a>
        </p>
    );
};

ReactDOM.render(<Main />, document.getElementById('root'));

Here is a Codepen of the app.

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

Ok, looks like a neatly written React. Let's see what I can do.

       <div className="main-content col-sm-12">
            <div className="left-sided-lg-top-otherwise col-lg-8 col-md-12 col-sm-12 col-xs-12">
                {loading
                    ? <Loading />
                    : <BigStory newsItems={newsItems[0]}/>
                }
                {loading
                    ? <Loading />
                    : <MediumStory newsItems={newsItems[1]}/>
                }
                {loading
                    ? <Loading />
                    : <MediumStory newsItems={newsItems[2]}/>
                }
                <div className="col-sm-4 col-xs-12">
                    {loading
                        ? <Loading />
                        : <SmallStory newsItems={newsItems[3]}/>
                    }
                    {loading
                        ? <Loading />
                        : <SmallStory newsItems={newsItems[4]}/>
                    }
                    {loading
                        ? <Loading />
                        : <SmallStory newsItems={newsItems[5]}/>
                    }
                </div>
                {loading
                    ? <Loading />
                    : <DatedListNoPics items={listNoPics}/>
                }
            </div>
            <div className="right-sided-lg-bottom-otherwise col-lg-4 col-md-12 col-sm-12 col-xs-12">
                {loading
                    ? <Loading />
                    : <DatedListWithPics items={listWithPics}/>
                }
            </div>
        </div>

Instead of hard-coding news 1 to 5, consider making them lists too (even if they're just 1 story). Lists are easier to manage and is "future-proof" in a sense that if you want to add more, you'll probably just tweak some constant to have it load more. Which brings us to the next piece...

// List 1 (no pics):
for (let i = 6; i <= 11; i++) {
    listNoPics.push(newsItems[i]);
}

// List 2 (with pics):
for (let i = 12; i <= storiesToShow; i++) {
    listWithPics.push(newsItems[i]);
}

An alternative way of building this (in a non-imperative way) is to use a range function, like the one from lodash and a map function to transform each value into another value. In this case, ranges into news lists. Recursive is ok, but with the overhead of building a recursive function (which is overkill).

var storyMapper  = (i) => newsItems[i];
var bigStory     = _.range(0, 1).map(storyMapper);
var mediumStory  = _.range(1, 3).map(storyMapper);
var smallStory   = _.range(3, 6).map(storyMapper);
var listNoPics   = _.range(6, 11).map(storyMapper);
var listWithPics = _.range(12, storiesToShow).map(storyMapper);

Now I mentioned earlier about a "configurable list of things". You can put the range arguments in some constant somewhere in a config. This allows you to easily adjust the lists.

If you want to create a range of your own, you can simply use Array.fill with array.map.

function range(start, end){
  return Array(end - start).fill(0).map((v,i) => start + i);
}

When your app becomes complex like this, naming becomes hard especially for CSS classes (and no, don't even consider inline styles). Check out BEM. It's an element naming convention that manages your CSS classes without having the styles stepping each other's feet. Take for example, your Main component.

<div class="layout-classes main">
  ...
  <div class="layout-classes main__container">
    ...
    <div class="layout-classes main__small-stories {loading ? 'main__small-stories--loading' : ''}>
      ...

.main{...}
.main__container{...}
.main__small-stories{...}
.main__small-stories--loading{display:none} // hides small stories until removed

With this naming convention, all your components will have a unique BEM name, essentially collision-free (be concise about the names though). Your CSS will all end up with a very low specificity of 0-1-0, making them easily overridable (goodbye !important). Besides, why would you override when you know they're unique to that component and can safely change them? (unless it's inherited from a parent)

I see you're using React, thus Babel which tells me that you have a build phase. Consider doing the same for your CSS by using a preprocessor like SASS or LESS. That way, you can use mixins. For instance, your BEM-ified main__small-stories could look like:

.main__small-stories{
  // styles for small stories ON MOBILE

  @include medium-screens{
    // styles for medium screens
  }

  @include large-screens{
    // styles for large screens
  }
}

So in the above, it uses "mixins" and just inverts the definition of media queries. Instead of starting off with a media query, and duplicating selectors, you define the selector and default styles, and "append" what happens on different screens. Output CSS is still the same, but from an authoring perspective, it's much readable.

\$\endgroup\$
  • \$\begingroup\$ I found this very useful, thank you. Can I ask is there any particular reason you're using var as opposed to let or const? \$\endgroup\$ – alanbuchanan Dec 28 '15 at 12:20
  • \$\begingroup\$ @alanbuchanan old habits die hard :D no particular reason really. \$\endgroup\$ – Joseph Dec 28 '15 at 12:47

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.