patternjavascriptMinor

React & ES6 news app

Submitted by: @import:stackexchange-codereview·Mar 10, 2026·

Viewed 0 times

es6newsreactapp

Problem

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 (

{/ Bootstrap nav /}

Toggle navigation

F
C
C

freecodecamp
bbc news
github

NEWS

);
};

const BigStory = (props) => {

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

headline = splitHeadlineAtUnwantedChar(headline);

return (

{newsItems.metaDescription}

);
};

const MediumStory = (props) => {

const {newsItems} = props;

newsItems.headline = splitHeadlineAtUnwantedChar(newsItems.headline);

return (

{newsItems.metaDescription}

);
};

const SmallStory = (props) => {

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

Solution

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

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.


  ...
  
    ...
    
      ...

.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.

Code Snippets

<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>

// 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]);
}

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);

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

<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

Context

StackExchange Code Review Q#114991, answer score: 3

Revisions (0)

No revisions yet.