Looking for feedbacks on Markdown Previewer React App

Code Feedback
1

Hi Everyone,

Made a React App - Markdown Previewer as part of FCC’s Front End Development Libraries Curriculum. Please, have a review and let me know the areas where I can improve upon.

Project Link: Live Markdown Previewer

Thanks,
Shraddha

(Huge thanks to @kevinSmith for helping me resolve few problems after putting the code in Codepen)

2 Likes
2

HI @mailmeatshraddha !

I think your page looks good.

If you are curious on completing the optional test then you can look to this part of the marked documentation.

https://marked.js.org/using_advanced

1 Like
3

Thanks @jwilkins.oboe . I am surely interested in completing the optional part. I will look into it and update you. Thanks again :+1:

4

Yeah, it looks pretty good - I like the retro feel.

As to the code, it looks pretty good too. There are a few things that caught my attention:

const editorMaximised = () => {
  return {
    type: EDITOR_MAXIMISED
  };
};

You’re going to see most people use the implicit return here, like:

const editorMaximised = () => ({
  type: EDITOR_MAXIMISED
});

// or

const editorMaximised = () => ({ type: EDITOR_MAXIMISED });

Note the parentheses around the return object - necessary because you can’t begin an expression with a curly base or it assumes it is a code block.

There are several places yout could do this, including things like Header and Footer.

This isn’t wrong per se, just that to some eyes it may seem a little dated and cumbersome.

This seems overkill to me:

  React.useEffect(() => {
    if (props.isMaximised) {
      setMaxClass("maximised");
    } else {
      setMaxClass("");
    }
  }, [props.isMaximised]);

Couldn’t you just have:

<div className={`markdownEditor ${props.isMaximised ? 'maximised' : ''}`}>

or even this might work (I can’t remember):

<div className={`markdownEditor ${props.isMaximised && 'maximised' }`}>

You could even store that in a variable to make your JSX cleaner:

const wrapperClassString = `markdownEditor ${props.isMaximised ? 'maximised' : ''}`;

You won’t need component state unless you need to persist it across component renders. I don’t think you need to here because it is just based on an input prop. In general, it’s good to make your components as dumb as possible, make them idempotent. Some components will need to have state, but avoid it where possible.

In general, there seem to be a few things here being handled in redux that could maybe be handled in local state. But I understand that you have a new shiny toy and want to use it. In reality, an app like this, I probably wouldn’t even bother with redux. The same with using state where you didn’t need it - it’s still a good learning opportunity.

      <React.Fragment>
        // ...
      </React.Fragment>

Now, this can be simplified to:

      <>
        // ...
      </>

Still, it looks good, all in all. Have fun on the drum machine.

2 Likes
5

Thanks for the review @kevinSmith. I really was looking for these detailed suggestions. I am learning ReactJs now as a beginner, so honest feedback helps me to understand where and how to improve. I will incorporate the changes suggested by you. I am glad that you took the time to go over my code and share your feedback. Thanks a lot!

Right now, I just resolved few of the responsiveness issues. I would now go ahead and look into the changes/ improvements suggested by you and @jwilkins.oboe.

1 Like
6

Yeah, React is weird and it takes a while to get all the nuances. But once you do, it’s incredibly powerful. And you didn’t really make “mistakes” per se, just little things where things could have been more streamlined.

1 Like
7

Thanks @jwilkins.oboe for the suggestion. I have incorporated the below code, to pass the optional test:

marked.setOptions({
  breaks: true,
});
2 Likes
8

This topic was automatically closed 182 days after the last reply. New replies are no longer allowed.